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

Make redefining const globals into an error #54114

Conversation

oscardssmith
Copy link
Member

This is an interesting mix of breaking and non-breaking. On the one hand, there are definitely people who do this in a REPL, but it is also UB, so by language lawyer standards, we are allowed to change it in a minor release. I think that since we have had typed globals since 1.8, this is something we should at least consider since typed globals allow a non-UB way of modifying a global that is still fast.

Tagging triage since there definitely will be a lot of thoughts on whether or not this is too breaking.

@oscardssmith oscardssmith added breaking This change will break code needs news A NEWS entry is required for this change triage This should be discussed on a triage call needs pkgeval Tests for all registered packages should be run with this change labels Apr 17, 2024
@adienes
Copy link
Contributor

adienes commented Apr 17, 2024

also potentially fair to break over multiple releases

e.g. in 1.12 maybe only the warning messages changes to state this warning will become an error in a future release

and then fully break in 1.13

@KristofferC
Copy link
Member

This would make the REPL awful!!!

@antoine-levitt
Copy link
Contributor

This definitely breaks a lot of workflows. If done at all it should be deprecated first, and with a clear replacement. Typed globals are not a complete replacement because you have to figure out the type of the rhs, which can sometimes be annoying. Something like a @consttype lhs=rhs which would turn into temp = rhs ; lhs::typeof(temp) = temp if lhs is undefined, and be a noop if lhs is already defined.

@jakobnissen
Copy link
Contributor

I would definitely prefer it to always error, rather than it being in a limbo state of "could possibly break at any time and/or cause miscompilations". I don't think there is much value in having it be permissable, but UB, such that I can't trust any result produced by a program that does it.

That being said, perhaps it would be a good idea to figure out what it would take to un-break the workflows that this PR breaks. For example, it's not clear to me why this would make the REPL experience worse. Would this also include redefining structs?

@adienes
Copy link
Contributor

adienes commented Apr 17, 2024

This definitely breaks a lot of workflows

sounds like they're already broken?

@mbauman
Copy link
Member

mbauman commented Apr 17, 2024

In many common usages, not only is the const re-defined, but all functions are, too. E.g.,

const a = 2
f(x) = a*x
g() = f(5)
@show g()

I can go back, change the value of a, and then re-evaluate that entire file. That works, and I have a mental model for what Julia is doing that gives me confidence that it should work. Maybe that mental model is wrong, or maybe a future Julia will skip re-defining methods that "should" not have new bodies (e.g., #21113), and if const redefinition is UB (cf. #54099) then that's totally fair game.

But right now, the behavior of Julia is consistent enough that folks rely on it.

@giordano
Copy link
Contributor

Something like a @consttype lhs=rhs

https://giordano.github.io/blog/2022-06-18-first-macro/

@KristofferC
Copy link
Member

For example, it's not clear to me why this would make the REPL experience worse.

Because it is common to paste in a unit of code that includes some const definitions and methods using those.

It's funny how some off-hand discussion about UB immidiately lead to people trying to find ways to make existing work flows that have worked well for 10+. years not be usable anymore. Maybe don't?

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Apr 17, 2024

Why? Who's it hurting? Reiterating many comments... often handy, if you also redefine all the functions everything works. What's the upside of disallowing?

@antoine-levitt
Copy link
Contributor

@typeconst is in #54117

https://giordano.github.io/blog/2022-06-18-first-macro/

Nice, thanks! I was already done writing it when I saw your comment, but it would definitely have saved me time to read it first :-)

@Keno
Copy link
Member

Keno commented Apr 18, 2024

We should just make this work properly (#40399). Making this an error will cause a riot.

@KristofferC
Copy link
Member

I'm just going to close this because seeing this open is very uncomfortable.

@LilithHafner LilithHafner removed the triage This should be discussed on a triage call label Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code needs news A NEWS entry is required for this change needs pkgeval Tests for all registered packages should be run with this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants