Make world-age increments explicit #56509

Keno · 2024-11-09T04:40:10Z

This PR introduces a new, toplevel-only, syntax form :worldinc that semantically represents the effect of raising the current task's world age to the latest world for the remainder of the current toplevel evaluation (that context being an entry to eval or a module expression). For detailed motivation on why this is desirable, see #55145, which I won't repeat here, but the gist is that we never really defined when world-age increments and worse are inconsistent about it. This is something we need to figure out now, because the bindings partition work will make world age even more observable via bindings.

Having created a mechanism for world age increments, the big question is one of policy, i.e. when should these world age increments be inserted.

Several reasonable options exist:

After world-age affecting syntax constructs (as proprosed in Make precise semantics of world-age increments in top-level thunks #55145)
Option 1 + some reasonable additional cases that people rely on
Before any top level call expression
Before any expression at toplevel whatsover

As an example, case, consider a == a at toplevel. Depending on the semantics that could either be the same as in local scope, or each of the four world age dependent lookups (three binding lookups, one method lookup) could (potentially) occur in a different world age.

The general tradeoff here is between the risk of exposing the user to confusing world age errors and our ability to optimize top-level code (in general, any :worldinc statement will require us to fully pessimize or recompile all following code).

This PR basically implements option 2 with the following semantics:

The interpreter explicit raises the world age only at :worldinc exprs or after :module exprs.
The frontend inserts :worldinc after all struct definitions, method definitions, using and `import.
The @eval macro inserts a worldinc following the call to eval if at toplevel
A literal (syntactic) call to include gains an implicit worldinc.

Of these the fourth is probably the most questionable, but is necessary to make this non-breaking for most code patterns. Perhaps it would have been better to make include a macro from the beginning (esp because it already has semantics that look a little like reaching into the calling module), but that ship has sailed.

Unfortunately, I don't see any good intermediate options between this PR and option #3 above. I think option #3 is closest to what we have right now, but if we were to choose it and actually fix the soundness issues, I expect that we would be destroying all performance of global-scope code. For this reason, I would like to try to make the version in this PR work, even if the semantics are a little ugly.

The biggest pattern that this PR does not catch is:

eval(:(f() = 1))
f()

We could apply the same include special case to eval, but given the existence of @eval which allows addressing this at the macro level, I decided not to. We can decide which way we want to go on this based on what the package ecosystem looks like.

Keno · 2024-11-09T08:25:39Z

The SparseArrays test failure is known - SparseArrays.allowscalar redefines indexing methods and the test relies on that getting picked up right away. Should be changed to a macro with a worldage increment probably, but before going through the hassle of the multi repo coordination, let's finish this PR.

Keno · 2024-11-09T08:25:48Z

@nanosoldier runtests()

aviatesk · 2024-11-09T11:54:26Z

I’m not opposed to this PR, but what’s the specific reason for introducing :worldinc? From what I understand from reading #55145, it seems like modifying the runtime to adhere to these rules would avoid the discussed issues?

We stop incrementing the world age for every statement. Instead, world age (at the toplevel) is incremented to the latest world age only at the following points:
a. Entry into toplevel evaluation (i.e., at the beginning of Core.eval)
b. After any evaluation of :method
c. On the bindings branch, after introducing a new binding

(plus an increment after a :module expression as per you mentioned above)

That said, I think introducing :worldinc would make it easier to fix inference-related problems, like ensuring world-age is incremented correctly for include as done in this PR. So I’m actually in favor of adding this expression type, but I’d like to understand the reasoning clearly.

aviatesk · 2024-11-09T11:55:56Z

test/reflection.jl

 @test faz1(1) == 2
 @test faz1(1.0) == 1
 m = first(methods(bar1, Tuple{Int}))
 Base.delete_method(m)


Does this mean that the result of Base.delete_method (and Base.delete_binding) won’t take effect until the user explicitly calls Core.@worldinc? That sounds like it would be a pretty breaking change. I agree with the idea of restricting the effect of :worldinc to the top level (for helping inference), but it seems like Base.delete_method might need some adjustments, such as calling @eval internally or something similar.

Does this mean that the result of Base.delete_method (and Base.delete_binding) won’t take effect until the user explicitly calls Core.@WorldInc?

It takes effect immediately (in a new world age), but the effect is not visible in the current task until it raises it world age.

Calling @eval internally will not raise the world age, because it's not at top level. delete_method is mostly used internally by Revise which can take an appropriate adjustment. There could also be a delete_method macro that does the world age raising implicitly.

Keno · 2024-11-10T02:20:38Z

but what’s the specific reason for introducing :worldinc?

Two reasons:

As a separation of concerns issue and to put the more complicated world-age increment decisions into lowering rather than forcing the logic to be duplicated in several places
To enable it to be inserted by macros that may want it

Keno · 2024-11-10T02:25:05Z

@vtjnash suggests implicit world-age increments between the args of :toplevel and :module. I think this is reasonable and I'll make an appropriate adjustment.

Keno · 2024-11-10T02:58:03Z

@vtjnash suggests implicit world-age increments between the args of :toplevel and :module.

Forgot to mention that @vtjnash pointed out that I already made world age increment for macro purposes in #53515, so it would be inconsistent for it not to increment the world age for execution, which I found convincing.

nanosoldier · 2024-11-10T03:37:42Z

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

Keno · 2024-11-10T03:44:29Z

@nanosoldier runtests(["Tricks", "MethodInspector", "ExprTools", "OptimizingIR", "OverflowContexts", "CBOOCall", "LazyModules", "PowerSeries", "Purses", "Hygienic", "MullerPlot", "SyntaxTree", "Plugins", "ParameterisedModule", "ApproximationAnalysis", "VectorInterface", "Expronicon", "TestExtras", "Hyperspecialize", "TimerOutputs", "Behavior", "ProblemSet", "FinitePosets", "CommandLiner", "TrixiBase", "IRTools", "TypeStability", "LoweredCodeUtils", "LispSyntax", "FileIO", "Automa", "ComoniconTargetExpr", "SplittablesBase", "HAML", "JSON3", "PDDL", "Continuables", "DataSets", "DataFlowTasks", "GraphQLGen", "Revise", "ConfigurationsENV", "DebugAdapter", "LIBSVM", "FromFile", "Groups", "LazyReports", "Polyester", "Handcalcs", "PkgJogger", "IntervalLinearAlgebra", "TypeClasses", "Open62541", "MHLib", "ExtensibleEffects", "Arblib", "PlutoVista", "LegendrePolynomials", "LinkedInAPI", "SwagUI", "GraphQLClient", "Folds", "Serde", "FlatRBAC", "BenchmarkProfiles", "DataToolkitCommon", "ACSets", "SphericalHarmonicExpansions", "CrystalInfoFramework", "Experimenter", "MATDaemon", "DrelTools", "DistributedSparseGrids", "TransitionalMCMC", "AstroRepresentations", "TransformSpecifications", "PlutoPlotly", "Bukdu", "OndaEDFSchemas", "AppleHealthParser", "MultivariateChebyshev", "Onda", "AlignedSpans", "YasolSolver", "JSXGraph", "Semagrams", "Gen", "MaterialDecomposition", "CSetAutomorphisms", "GeniePlugins", "GenieSession", "GeniePackageManager", "GenieCache", "GenieDeployHeroku", "GenieCacheFileCache", "GenieDeployDocker", "GenieSessionFileSession", "InfiniteOpt", "EnergyModelsBase", "EnergyModelsCO2", "EnergyModelsRenewableProducers", "EnergyModelsGeography", "OndaEDF", "MembraneBase", "Books", "StippleMathjs", "Stipple", "StippleUI", "SparseArrayKit", "QWignerSymbols", "RegNets", "GenieDevTools", "HerbSearch", "Equate", "StippleCharts", "Herb", "StipplePlotly", "SteadyStateDiffEq", "StipplePlotlyExport", "TaylorInversion", "GenieAuthorisation", "CategoryData", "GenieFramework", "ReversePropagation", "GivEmXL", "GenericCharacterTables", "SphericalFunctions", "StippleKeplerGL", "Spehulak", "PlutoPages", "GeometryOptimization", "ProToPortal", "JuliaBUGS", "MLJTestIntegration", "CausalGPSLC", "MLJ", "WaveSpec", "Individual", "AntennaPattern", "SimpleCrop", "UnfoldBIDS", "LeafGasExchange", "Garlic", "EMpht", "Vahana", "Petri", "Lighthouse", "AffineMotions", "PlantGeomTurtle", "HetaSimulator", "Serialization", "CropRootBox"])

nanosoldier · 2024-11-10T05:33:05Z

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

Keno · 2024-11-11T00:20:57Z

Alright, a fair number of tests with dependencies on JuliaInterpreter or Revise, which need to be taught about :worldinc, but also some that noticed the behavior change. I think the thing to do is

an independent PR that just introduces :worldinc, but doesn't use it yet
Fix JuliaInterpreter/Revise
Re-run PkgEval and see what to do about the rest of the packages

Split out from #56509 to facilitate adjusting downstream packages.

This was part of #56509, but is an independent bugfix. The basic issue is that these macro were using `do` block internally. This is undesirable for test macros, because we would like them not to affect the behavior of what they're testing. E.g. right now: ``` julia> using Test julia> const x = 1 1 julia> @test_nowarn const x = 1 ERROR: syntax: `global const` declaration not allowed inside function around /home/keno/julia/usr/share/julia/stdlib/v1.12/Test/src/Test.jl:927 Stacktrace: [1] top-level scope @ REPL[3]:1 ``` This PR just writes out the try/finally manually, so the above works fine after this PR.

Split out from #56509 to facilitate adjusting downstream packages.

vtjnash · 2024-11-13T19:16:51Z

stdlib/REPL/src/REPL.jl

                for xf in backend.ast_transforms
                    ast = Base.invokelatest(xf, ast)
                end
+                toplevel_eval_with_hooks(mod, Expr(:worldinc))


This is a no-op (if the implementation is not broken)

Suggested change

toplevel_eval_with_hooks(mod, Expr(:worldinc))

vtjnash

Overall SGTM (though needs a rebase against the parts that are merged already).

Looks like a lot of the needed added Core.@worldinc also will be already solved by making each toplevel / module statement continue to imply an updated world (since they already imply an updated macro world, and the statements are supposed to run in the world after that macro expansion, which wouldn't be possible to express with these explicit markers anyways)

One other remaining todo item is adding this to the Compiler verifier code. I don't know if we run any debug tests on PRs, but I believe we run those on nightly, and it likely should error on the unexpected Expr head.

These were added in JuliaLang/julia#56523. This simply adds tracking for the world age to the interpreter, but does not use it for anything. JuliaInterpreter's current model of world age does not match Base anyway. We should fix that eventually, but it's probably easier to do that once JuliaLang/julia#56509 and binding partitions are merged.

* Handle `:latestworld` expr These were added in JuliaLang/julia#56523. This simply adds tracking for the world age to the interpreter, but does not use it for anything. JuliaInterpreter's current model of world age does not match Base anyway. We should fix that eventually, but it's probably easier to do that once JuliaLang/julia#56509 and binding partitions are merged. * 1.6 compat

Upcoming base PR JuliaLang/julia#56509 makes precise when exactly world age increments automatically at top-level. Packages are mostly not supposed to notice, but of course Revise is a bit patholgical in that it does basically expect asynchronous changes to have top level effects. After that PR, that requires an explicit opt-in. This PR makes the appropriate adjustments to the tests.

Keno · 2024-11-18T03:33:23Z

@nanosoldier runtests(["Tricks", "MethodInspector", "ExprTools", "OptimizingIR", "OverflowContexts", "CBOOCall", "LazyModules", "PowerSeries", "Purses", "Hygienic", "MullerPlot", "SyntaxTree", "Plugins", "ParameterisedModule", "ApproximationAnalysis", "VectorInterface", "Expronicon", "TestExtras", "Hyperspecialize", "TimerOutputs", "Behavior", "ProblemSet", "FinitePosets", "CommandLiner", "TrixiBase", "IRTools", "TypeStability", "LoweredCodeUtils", "LispSyntax", "FileIO", "Automa", "ComoniconTargetExpr", "SplittablesBase", "HAML", "JSON3", "PDDL", "Continuables", "DataSets", "DataFlowTasks", "GraphQLGen", "Revise", "ConfigurationsENV", "DebugAdapter", "LIBSVM", "FromFile", "Groups", "LazyReports", "Polyester", "Handcalcs", "PkgJogger", "IntervalLinearAlgebra", "TypeClasses", "Open62541", "MHLib", "ExtensibleEffects", "Arblib", "PlutoVista", "LegendrePolynomials", "LinkedInAPI", "SwagUI", "GraphQLClient", "Folds", "Serde", "FlatRBAC", "BenchmarkProfiles", "DataToolkitCommon", "ACSets", "SphericalHarmonicExpansions", "CrystalInfoFramework", "Experimenter", "MATDaemon", "DrelTools", "DistributedSparseGrids", "TransitionalMCMC", "AstroRepresentations", "TransformSpecifications", "PlutoPlotly", "Bukdu", "OndaEDFSchemas", "AppleHealthParser", "MultivariateChebyshev", "Onda", "AlignedSpans", "YasolSolver", "JSXGraph", "Semagrams", "Gen", "MaterialDecomposition", "CSetAutomorphisms", "GeniePlugins", "GenieSession", "GeniePackageManager", "GenieCache", "GenieDeployHeroku", "GenieCacheFileCache", "GenieDeployDocker", "GenieSessionFileSession", "InfiniteOpt", "EnergyModelsBase", "EnergyModelsCO2", "EnergyModelsRenewableProducers", "EnergyModelsGeography", "OndaEDF", "MembraneBase", "Books", "StippleMathjs", "Stipple", "StippleUI", "SparseArrayKit", "QWignerSymbols", "RegNets", "GenieDevTools", "HerbSearch", "Equate", "StippleCharts", "Herb", "StipplePlotly", "SteadyStateDiffEq", "StipplePlotlyExport", "TaylorInversion", "GenieAuthorisation", "CategoryData", "GenieFramework", "ReversePropagation", "GivEmXL", "GenericCharacterTables", "SphericalFunctions", "StippleKeplerGL", "Spehulak", "PlutoPages", "GeometryOptimization", "ProToPortal", "JuliaBUGS", "MLJTestIntegration", "CausalGPSLC", "MLJ", "WaveSpec", "Individual", "AntennaPattern", "SimpleCrop", "UnfoldBIDS", "LeafGasExchange", "Garlic", "EMpht", "Vahana", "Petri", "Lighthouse", "AffineMotions", "PlantGeomTurtle", "HetaSimulator", "Serialization", "CropRootBox"])

Goes along with JuliaLang/julia#56509

This test currently relies on implicit world age increments at top level. We're re-evaluating where these go because julia is currently inconsistent about it in the interpreter, compiler and inference. To make sure this test keeps working on 1.12, add an explicit world age increment. See JuliaLang/julia#56509.

* Adapt to JuliaLang/julia#56509 * Adapt to JuliaLang/julia#54734 * Use StmtRange explicitly * Adapt to JuliaLang/julia#57230 * Reuse Cthulhu code structure for Compiler cache/finish overrides * Adapt to JuliaLang/julia#57475 * Adapt to JuliaLang/julia#55976 * Adapt to JuliaLang/julia#54734 * Use CC instead of .Compiler * Implement ir.argtypes[1] fix from JuliaLang/julia#54458 * Comment out failing tests To highlight which are broken, should probably be fixed before merging * Treat `getproperty(::Module, ::Symbol)` like GlobalRefs * Uncomment passing tests, explicitly mark others as broken * Evaluate GlobalRef only if binding is defined * Use `rrule` for getproperty(::Module, ::Symbol) * Bump compat bound for StructArrays * Raise compat bound for Cthulhu * Revert `isconst` change now that it is fixed * Adapt to `finishinfer!` signature change --------- Co-authored-by: Cédric Belmant <[email protected]>

For method defs, `latestworld` is produced in desugaring rather than closure conversion for now (our closure conversion doesn't seem to cover the same cases as lisp lowering yet). Covers JuliaLang/julia#56523, JuliaLang/julia#56509, JuliaLang/julia#57299. Also includes changes from JuliaLang/julia#57102 (bpart: Start enforcing minimum world age for const bparts) and JuliaLang/julia#57150 (bpart: Start enforcing min_world for global variable definitions) since the lowering changes from those appear to be amendments to the changes above.