Don't let setglobal! implicitly create bindings #54678
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Keno
added
needs pkgeval
vtjnash
reviewed
Jun 5, 2024
test/syntax.jl
Outdated
| end | ||
| baz = 4 | ||
| begin; | ||
| for i = 1:10; end |
There was a problem hiding this comment.
Suggested change
| for i = 1:10; end | |
| Base.Experimental.@force_compile |
There was a problem hiding this comment.
Ah, I thought we had this one, I just forgot what it was called.
|
@nanosoldier |
JeffBezanson
approved these changes
Jun 5, 2024
|
The package evaluation job you requested has completed - possible new issues were detected. |
Keno
added a commit
to Keno/Toolips.jl
that referenced
this pull request
Jun 6, 2024
The `mod.sym = val` syntax is not supposed to be able to create new bindings if `mod.sym` does not yet exist. The error check for this was accidentally dropped in Julia 1.9, but will likely be put back in 1.11 [1]. This uses Core.eval/invokelatest to achieve the same effect using the recommeded replacement. As a general note, the way this package uses bindings is clever, but not particularly idiomatic in Julia. Not saying you can't do it, but I would recommend revisiting if at least the injected bindings could perhaps be replaced by something like an IdDict. [1] JuliaLang/julia#54678
Keno
added a commit
to Keno/RTableTools.jl
that referenced
this pull request
Jun 6, 2024
See JuliaLang/julia#54678, `Mod.sym = val` is only supposed to work for globals that were previously declared, but the error message for this was accidentally dropped in Julia 1.9 and will likely be put back in Julia 1.11. As a general note though, you may want to use `const` bindings here rather than `global`s, as the compiler optimizes `const` better.
Keno
added a commit
to Keno/ACSets.jl
that referenced
this pull request
Jun 6, 2024
The `@intertypes` macro assigns `mod.Meta` without declaring it. This was accidentally allowed in Julia 1.9/1.10 due to a dropped error check, but will likely be disallowed again in Julia 1.11. See JuliaLang/julia#54678. This just declares it `global` (which is what happened implicitly on Julia 1.10/1.11). That said, you may want a `Const` instead, in which case you would need a different fix.
|
I've submitted PRs to Toolips, RTableTools and ACSets which should cover all the PkgEval failures, except the one in AlgebraicRelations, which appears to be a lowering bug that I have yet to investigate. |
|
|
From my perspective this is ready to go once #54701 is merged. |
PR #44231 (part of Julia 1.9) introduced the ability to modify globals with `Mod.sym = val` syntax. However, the intention of this syntax was always to modify *existing* globals in other modules. Unfortunately, as implemented, it also implicitly creates new bindings in the other module, even if the binding was not previously declared. This was not intended, but it's a bit of a syntax corner case, so nobody caught it at the time. After some extensive discussions and taking into account the near future direction we want to go with bindings (#54654 for both), the consensus was reached that we should try to undo the implicit creation of bindings (but not the ability to assign the *value* of globals in other modules). Note that this was always an error until Julia 1.9, so hopefully it hasn't crept into too many packages yet. We'll see what pkgeval says. If use is extensive, we may want to consider a softer removal strategy. Across base and stdlib, there's two cases affected by this change: 1. A left over debug statement in `precompile` that wanted to assign a new variable in Base for debugging. Removed in this PR. 2. Distributed wanting to create new bindings. This is a legimitate use case for wanting to create bindings in other modules. This is fixed in JuliaLang/Distributed.jl#102. As noted in that PR, the recommended replacement where implicit binding creation is desired is: ``` Core.eval(mod, Expr(:global, sym)) invokelatest(setglobal!, mod, sym, val) ``` The `invokelatest` is not presently required, but may be needed by #54654, so it's included in the recommendation now. Fixes #54607
Keno
added
merge me
KristofferC
pushed a commit
that referenced
this pull request
Jun 13, 2024
PR #44231 (part of Julia 1.9) introduced the ability to modify globals with `Mod.sym = val` syntax. However, the intention of this syntax was always to modify *existing* globals in other modules. Unfortunately, as implemented, it also implicitly creates new bindings in the other module, even if the binding was not previously declared. This was not intended, but it's a bit of a syntax corner case, so nobody caught it at the time. After some extensive discussions and taking into account the near future direction we want to go with bindings (#54654 for both), the consensus was reached that we should try to undo the implicit creation of bindings (but not the ability to assign the *value* of globals in other modules). Note that this was always an error until Julia 1.9, so hopefully it hasn't crept into too many packages yet. We'll see what pkgeval says. If use is extensive, we may want to consider a softer removal strategy. Across base and stdlib, there's two cases affected by this change: 1. A left over debug statement in `precompile` that wanted to assign a new variable in Base for debugging. Removed in this PR. 2. Distributed wanting to create new bindings. This is a legimitate use case for wanting to create bindings in other modules. This is fixed in JuliaLang/Distributed.jl#102. As noted in that PR, the recommended replacement where implicit binding creation is desired is: ``` Core.eval(mod, Expr(:global, sym)) invokelatest(setglobal!, mod, sym, val) ``` The `invokelatest` is not presently required, but may be needed by #54654, so it's included in the recommendation now. Fixes #54607 (cherry picked from commit b7e7232)
aviatesk
added a commit
to JuliaDebug/JuliaInterpreter.jl
that referenced
this pull request
Jun 14, 2024
KristofferC
added a commit
that referenced
this pull request
Jun 25, 2024
Backported PRs: - [x] #54361 <!-- [LBT] Upgrade to v5.9.0 --> - [x] #54474 <!-- Unalias source from dest in copytrito --> - [x] #54548 <!-- Fixes for bitcast bugs with LLVM 17 / opaque pointers --> - [x] #54191 <!-- make `AbstractPipe` public --> - [x] #53402 <!-- Add `jl_getaffinity` and `jl_setaffinity` --> - [x] #53356 <!-- Rename at-scriptdir project argument to at-script and search upwards for Project.toml --> - [x] #54545 <!-- typeintersect: fix incorrect innervar handling under circular env --> - [x] #54586 <!-- Set storage class of julia globals to dllimport on windows to avoid auto-import weirdness. Forward port of #54572 --> - [x] #54587 <!-- Accomodate for rectangular matrices in `copytrito!` --> - [x] #54617 <!-- CLI: Use `GetModuleHandleExW` to locate libjulia.dll --> - [x] #54605 <!-- Allow libquadmath to also fail as it is not available on all systems --> - [x] #54634 <!-- Fix trampoline assembly for build on clang 18 on apple silicon --> - [x] #54635 <!-- Aggressive constprop in trevc! to stabilize triangular eigvec --> - [x] #54645 <!-- ensure we set the right value to gc_first_tid --> - [x] #54554 <!-- make elsize public --> - [x] #54648 <!-- Construct LazyString in error paths for tridiag --> - [x] #54658 <!-- fix missing uuid check on extension when finding the location of an extension --> - [x] #54594 <!-- Switch to Pkg mode prompt immediately and load Pkg in the background --> - [x] #54669 <!-- Improve error message in inplace transpose --> - [x] #54671 <!-- Add boundscheck in bindingkey_eq to avoid OOB access due to data race --> - [x] #54672 <!-- make: Fix `sed` command for LLVM libraries with no symbol versioning --> - [x] #54624 <!-- more precise aliasing checks for SubArray --> - [x] #54679 <!-- 🤖 [master] Bump the Distributed stdlib from 6a07d98 to 6c7cdb5 --> - [x] #54604 <!-- Fix tbaa annotation on union selector bytes inside of structs --> - [x] #54690 <!-- Fix assertion/crash when optimizing function with dead basic block --> - [x] #54704 <!-- LazyString in reinterpretarray error messages --> - [x] #54718 <!-- fix prepend StackOverflow issue --> - [x] #54674 <!-- Reimplement dummy pkg prompt as standard prompt --> - [x] #54737 <!-- LazyString in interpolated error messages involving types --> - [x] #54642 <!-- Document GenericMemory and AtomicMemory --> - [x] #54713 <!-- make: use `readelf` for LLVM symbol version detection --> - [x] #54760 <!-- REPL: improve prompt! async function handler --> - [x] #54606 <!-- fix double-counting and non-deterministic results in `summarysize` --> - [x] #54759 <!-- REPL: Fully populate the dummy Pkg prompt --> - [x] #54702 <!-- lowering: Recognize argument destructuring inside macro hygiene --> - [x] #54678 <!-- Don't let setglobal! implicitly create bindings --> - [x] #54730 <!-- Fix uuidkey of exts in fast path of `require_stdlib` --> - [x] #54765 <!-- Handle no-postdominator case in finalizer pass --> - [x] #54591 <!-- Don't expose guard pages to malloc_stack API consumers --> - [x] #54755 <!-- [TOML] remove Dates hack, replace with explicit usage --> - [x] #54721 <!-- add try/catch around scheduler to reset sleep state --> - [x] #54631 <!-- Avoid concatenating LazyString in setindex! for triangular matrices --> - [x] #54322 <!-- effects: add new `@consistent_overlay` macro --> - [x] #54785 - [x] #54865 - [x] #54815 - [x] #54795 - [x] #54779 - [x] #54837 Contains multiple commits, manual intervention needed: - [ ] #52694 <!-- Reinstate similar for AbstractQ for backward compatibility --> - [ ] #54649 <!-- Less restrictive copyto! signature for triangular matrices --> Non-merged PRs with backport label: - [ ] #54779 <!-- make recommendation clearer on manifest version mismatch --> - [ ] #54739 <!-- finish implementation of upgradable stdlibs --> - [ ] #54738 <!-- serialization: fix relocatability bug --> - [ ] #54574 <!-- Make ScopedValues public --> - [ ] #54457 <!-- Make `String(::Memory)` copy --> - [ ] #53957 <!-- tweak how filtering is done for what packages should be precompiled --> - [ ] #53452 <!-- RFC: allow Tuple{Union{}}, returning Union{} --> - [ ] #53286 <!-- Raise an error when using `include_dependency` with non-existent file or directory --> - [ ] #51479 <!-- prevent code loading from lookin in the versioned environment when building Julia -->
KristofferC
removed
the
backport 1.11
davidanthoff
added a commit
to julia-vscode/TestItemRunner2.jl
that referenced
this pull request
Jul 4, 2024
96ed09c version 0.9.32 0a8da3a minimum adjustments to make JuliaInterpreter work with v1.12 (#631) 397ea70 `public` as an identifier is deprecated (#612) ce6e341 update builtins.jl (#628) 1024848 Merge pull request #629 from JuliaDebug/avi/54678 5700dcc adjustments to JuliaLang/julia#54428 ea522b8 adjustments to JuliaLang/julia#54678 eae3b4c Some more 1.12 compat (#625) fc4aeca version 0.9.31 a265c14 [incomplete] changes needed to adapt to compressed line table format in Julia (#606) 31253a0 version 0.9.30 1b1b6d6 adjustments for Julia v1.11 (#615) 8043dbc add support for preset `current_scope` (#619) 55e33d0 use child `@testset` to get nicer test summary (#618) e0e34be adapt to implicit leave change in Julia (#614) f7138f9 v0.9.29 82b1552 Adjust to upcoming compiler change (#611) 011edf9 improve test code 78f766b minor improvements 713c768 implement support for `current_scope` (#605) 580b95c version 0.9.28 d7d4ced Fix revise#718 (#609) 0089e4b update docs and convert `jldoctest` to `@repl` blocks (#607) d319168 Remove buggy linearization pass (#604) 1efae18 remove `AbstractFrameInstance` (#608) 0138e60 update CI.yml 6b1c476 version 0.9.27 ce20820 adjust to the `:enter` IR changes made in JuliaLang/julia#52300 (#599) 9afdf71 follow up #596 (#600) 9d50726 adapt to array changes in Julia base (#596) 68fa8be NFC: harden some internal ccalls (#595) ccc1c95 remove old compats (#598) 15ad1c7 set CI timeout (#597) 7beca92 version 0.9.26 a0d0d33 Adjust to upcoming julia lowering change (#592) a3cf18e Add a second link to the docs from the README (#589) 6da0b26 version 0.9.25 c8d1ef7 adjust to JuliaLang/julia#50943 (#585) c93dedf adjust tests for latest Julia master (#584) 910cb6f Align arguments number in breakpoints hook docstring (#583) 7849d4a Bump actions/checkout from 2 to 3 (#576) 0169df2 Version 0.9.24 14e454b Ignore `:aliasscope` and `:popaliasscope` (#581) 3ab2674 Bump codecov/codecov-action from 1 to 3 (#577) cc1bace Bump actions/cache from 1 to 3 (#578) 1d87867 Update some failing doctests (#579) 43f2041 enable dependabot for GitHub actions (#575) aefaa30 use an explicit Any comprehension (#572) 475512b Version 0.9.23 8fecf35 Fix kw pattern matching, other changes on 1.9+ (#568) cf7f437 also test on 1.9 57dbc98 version 0.9.22 d7a3dd4 fixup b4d133d adjust to JuliaLang/julia#48693 (#566) 9026819 fix test on nightly (#564) 9c5454c Protect `error` calls from invalidation (#565) da3fee2 remove unused import 2a1c076 bump version 6d2fbaf rejigger the code to compute the method instance in stacktraces (#563) git-subtree-dir: packages/JuliaInterpreter git-subtree-split: 96ed09c7127475d391b1a4f20906072f482278eb
j-fu
added a commit
to j-fu/ExtendableGrids.jl
that referenced
this pull request
Sep 27, 2024
- Seems to be disallowed now by JuliaLang/julia#54678 - Seems to be not necessary as calls before already did this via the gmsh API
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR #44231 (part of Julia 1.9) introduced the ability to modify globals with
Mod.sym = valsyntax. However, the intention of this syntax was always to modify existing globals in other modules. Unfortunately, as implemented, it also implicitly creates new bindings in the other module, even if the binding was not previously declared. This was not intended, but it's a bit of a syntax corner case, so nobody caught it at the time.After some extensive discussions and taking into account the near future direction we want to go with bindings (#54654 for both), the consensus was reached that we should try to undo the implicit creation of bindings (but not the ability to assign the value of globals in other modules). Note that this was always an error until Julia 1.9, so hopefully it hasn't crept into too many packages yet. We'll see what pkgeval says. If use is extensive, we may want to consider a softer removal strategy.
Across base and stdlib, there's two cases affected by this change:
precompilethat wanted to assign a new variable in Base for debugging. Removed in this PR.As noted in that PR, the recommended replacement where implicit binding creation is desired is:
The
invokelatestis not presently required, but may be needed by #54654, so it's included in the recommendation now.Fixes #54607