Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix error message when using const inside a function, require the LHS to be global, and prohibit "const x[] = 1" #57470

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

xal-0
Copy link
Member

@xal-0 xal-0 commented Feb 19, 2025

The changes to const resulted in confusing error messages when it was used
inside a function (#57334). On 1.11.3:

julia> function f()
           const x = 1
       end
ERROR: syntax: unsupported `const` declaration on local variable around REPL[1]:2
Stacktrace:
 [1] top-level scope
   @ REPL[1]:1

On nightly:

julia> function f()
           const x = 1
       end
ERROR: syntax: World age increment not at top level
Stacktrace:
 [1] top-level scope
   @ REPL[1]:1

In prior versions, we also accepted confused expressions like:

x = Ref(1)
const x[] = 1

This change adds a new error messages explicitly prohibiting const where the
left hand side is not declaring variables:

ERROR: syntax: `const` left hand side "x[]" contains non-variables around REPL[2]:1
Stacktrace:
 [1] top-level scope
   @ REPL[2]:1

Finally, #54773 made const stop participating in scope resolution (the left
hand side was always taken to be in global scope). Some expressions that were
prohibited started being accepted:

In 1.11.3:

julia> let
           const x = 1
       end
ERROR: syntax: unsupported `const` declaration on local variable around REPL[1]:2
Stacktrace:
 [1] top-level scope
   @ REPL[1]:1

Nightly:

julia> let
           const x = 1
       end
1

This change rejects const unless the variable would be in global scope
(global const would be required in the example), so we don't lose the
ability to make const in local scope meaningful later.

Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Minor nit with this error though:

julia> const (x{T},) = T
ERROR: syntax: `const` left hand side "SSAValue(81)" contains non-variables around REPL[13]:1

@xal-0 xal-0 added compiler:lowering Syntax lowering (compiler front end, 2nd stage) bugfix This change fixes an existing bug error messages Better, more actionable error messages labels Feb 19, 2025
@xal-0
Copy link
Member Author

xal-0 commented Feb 19, 2025

Argh, this is actually a regression.

1.11.3:

julia> const X{T}, Y{U} = [Int, Int]
2-element Vector{DataType}:
 Int64
 Int64

This:

julia> const X{T}, Y{U} = [Int, Int]
ERROR: syntax: `const` left hand side "SSAValue(4)" contains non-variables around REPL[1]:1
Stacktrace:
 [1] top-level scope
   @ REPL[1]:1

I'll need to think of a way to handle this.

@xal-0 xal-0 marked this pull request as draft February 22, 2025 00:34
xal-0 added a commit to xal-0/julia that referenced this pull request Feb 22, 2025
Also update some tests that used @test with a quote containing `const`, which
cannot be spliced into a function.
@xal-0 xal-0 force-pushed the const-function-errors branch from 33184ba to b45c8e0 Compare February 22, 2025 02:25
xal-0 added a commit to xal-0/julia that referenced this pull request Feb 24, 2025
Also update some tests that used @test with a quote containing `const`, which
cannot be spliced into a function.
@xal-0 xal-0 force-pushed the const-function-errors branch from b45c8e0 to a4f7d00 Compare February 24, 2025 19:21
xal-0 added a commit to xal-0/julia that referenced this pull request Feb 24, 2025
Also update some tests that used @test with a quote containing `const`, which
cannot be spliced into a function.
@xal-0 xal-0 force-pushed the const-function-errors branch from a4f7d00 to ce76dbf Compare February 24, 2025 19:42
@xal-0
Copy link
Member Author

xal-0 commented Feb 24, 2025

This is a note to myself to not squash 9fe9bbe so I am not git blame'd for all of the assignment lowering code.

@xal-0
Copy link
Member Author

xal-0 commented Feb 24, 2025

@nanosoldier runtests()

@vtjnash vtjnash added the don't squash Don't squash merge label Feb 25, 2025
@mlechu
Copy link
Contributor

mlechu commented Feb 26, 2025

Linking #25642

@nanosoldier
Copy link
Collaborator

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

Report summary

❗ Packages that crashed

1 packages crashed only on the current version.

  • An internal error was encountered: 1 packages

13 packages crashed on the previous version too.

✖ Packages that failed

24 packages failed only on the current version.

  • Package fails to precompile: 1 packages
  • Package has test failures: 1 packages
  • Package tests unexpectedly errored: 1 packages
  • Test duration exceeded the time limit: 21 packages

1109 packages failed on the previous version too.

✔ Packages that passed tests

27 packages passed tests only on the current version.

  • Other: 27 packages

5330 packages passed tests on the previous version too.

~ Packages that at least loaded

7 packages successfully loaded only on the current version.

  • Other: 7 packages

2926 packages successfully loaded on the previous version too.

➖ Packages that were skipped altogether

899 packages were skipped on the previous version too.

@KristofferC
Copy link
Member

@@ -3126,7 +3152,7 @@
((eq? (car e) 'assign-const-if-global)
(if (eq? (var-kind (cadr e) scope) 'local)
(if (length= e 2) (null) `(= ,@(cdr e)))
Copy link
Contributor

@mlechu mlechu Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
(if (length= e 2) (null) `(= ,@(cdr e)))
(if (length= e 2) (null) (resolve-scopes- `(= ,@(cdr e)) scope sp loc))

would fix #57574 (suggested here since you've already done this to the other branch).

xal-0 added 3 commits March 14, 2025 14:26
Also update some tests that used @test with a quote containing `const`, which
cannot be spliced into a function.
Modifies the handling of the 3-argument "lowered const" so `const x = y`
participates in scope resolution again (JuliaLang#56613); we treat (const x y) as an
assignment form, producing the "unsupported `const` declaration on local
variable" error if the usual scope rules would not make x global.

At the top level, these should succeed:

    const x = 1
    begin
        const x = 1
    end
    let
        const global x = 1
    end

Each of the following should fail:

    # ERROR: syntax: unsupported `const` declaration on local variable around REPL[1]:2
    let
        const x = 1
    end

    # ERROR: syntax: unsupported `const` declaration on local variable around REPL[1]:2
    function f()
        const x = 1
    end

This also amkes the use of `const global` inside a function body produce the
original, specific error message rather than a generic "World age increment not
at top level" error (JuliaLang#57334):

    # ERROR: syntax: `global const` declaration not allowed inside function around REPL[1]:2
    function f()
        global const x = 1
    end

Issue JuliaLang#57334 revealed we silently accept `const` expressions writing to
variables.  These now fail with a new error message:

    a = [1, 2]
    # ERROR: syntax: cannot declare "a[1]" `const` around REPL[1]:2
    const a[1] = 3

The bulk of these changes track const-ness through the functions called by
expand-assignment, since each of the three types of LHS is affect differently:
- ordinary variables become `const`, producing error if not global
- `X{T} = ...` forces X to be `const`, rather than the defaulting to being
  implicitly const only if in global scope.
- `f() = ...` ignores `const`.
@xal-0 xal-0 force-pushed the const-function-errors branch from ce76dbf to 14c14f1 Compare March 14, 2025 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug compiler:lowering Syntax lowering (compiler front end, 2nd stage) don't squash Don't squash merge error messages Better, more actionable error messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants