-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Add @typeconst #54117
base: master
Are you sure you want to change the base?
Add @typeconst #54117
Conversation
base/util.jl
Outdated
ex isa Expr || return ex | ||
if ex.head === :(=) | ||
quote | ||
tmp = $(esc(ex.args[2])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I wrote in https://giordano.github.io/blog/2022-06-18-first-macro/, you want to mark tmp
as local to avoid leaking the name to the calling namespace
tmp = $(esc(ex.args[2])) | |
local tmp = $(esc(ex.args[2])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I figured it was OK because it was gensymed (I had a previous version that called gensym() directly), but you're right it's even better not to leak it at all!
This macro is perhaps better suited for a package. See also #44259 as well as this old comment by you: #43671 (comment) |
As long as |
I don't get it. Why replace const? |
Const is very scary, and gives you absolutely no correctness guarantee in both in its docstring and in warnings, so it'd be good to have something less scary. I've asked before (I don't remember where) whether there was a path towards making const less scary, but that didn't seem like a likely prospect at the time. |
Just saying, the premise that this PR seems to build on (trying to replace consts with non-consts) is not gonna fly very far. I would suggest making it solely about a convenience of making a global with the type of an RHS and remove the references to |
How is const scary? If your program runs without warnings then there are no worries. If your program runs and has warnings then yes, you should be concerned, but that is always true, not just of const. Can you explain what your actual worry about using const is? |
"In some cases changing the value of a How is that not scary? By the way, the docstring doesn't mention that redefining const variables (eg Also, redefining Also also, every so often somebody comes up with even scarier things like redefining const is illegal and let's forbid it altogether. But of course my main issue with const is that it's terrible semantics. People could be forgiven for thinking that const are actually, you know, constant. Abusing this purely for performance is fundamentally a hack. |
This really feels like trying to manufacture fear, uncertainty and doubt where there isn't any real reason for it. The bottom line is: if your code emits warnings then you should be concerned, if it doesn't emit warnings then you do not need to worry. If you don't like being able to redefine constants, don't do it and you can forget about this entirely. There's absolutely nothing wrong with using |
Where is this coming from? Nobody has ever suggested abusing |
I understand people object to the wording of the news, I've changed it. To be clear, I'm talking about the use of
and then I was motivated by #54114 and the lack of appetite for making const redefinition be an "officially supported" thing to try to improve the situation, hence this PR. I really don't understand the situation here. It's certainly not my intention to manufacture FUD but to try to remove some. Is const redefinition supported with well-defined semantics, and will it continue to be for the foreseeable future? If yes, can we make its docstring less scary and vague (while still keeping the warning), something like "const redefinition is supported to help interactive usage, but it is the user's responsability to ensure that any function that uses the const global is redefined"? If no, shouldn't we warn users away from it more vehemently and design replacements? |
Please ignore #54114. The parties responsible have been flogged. We are not going to make redefining constants illegal. If you also redefine all the functions that rely on a constant, as you do in the workflow you're describing, then it works reliably. A more likely direction is that #40399 will happen—I'm pretty sure this is going to happen as soon as someone gets around to it—at which point redefining a constant will become less scary: code that was compiled to rely on the old value will continue to work with the old value, while newer code will use the new value. This will eliminate any "undefined behavior" whether functions are redefined or not. Constants will cease to be "constant" and simply be bindings that are very expensive to redefine. |
That would be awesome, and in that case there would be a lot less need for this PR. Let's see how that develops. |
This PR is good though— |
I do think the name of the macro needs to change. It has nothing to do with So maybe |
Eh, the type is constant 🤷🏻♂️. Kinda makes sense... |
I think a separate package would be a better place for this macro. Just this week I got bitten again by needing to find out which version of Compat I needed to support a certain feature on the LTS. If I had needed Your package would likely be about as small as UnPack. IMO, this is not a bad thing. It only means that the hurdle of adding the package as a dependency is very low; similar to the >180 packages using UnPack. |
I don't see why moving to a package would be better for such a simple thing. It can be added to Compat easily and it's not meant to be something depended on by packages, more for interactive use - I considered moving it to InteractiveUtils, but figured it'd be confusing to people if it worked at the REPL and not in a package. |
It would be cute if this could be written as something like |
I agree typeconst is too big, that's why I added support for
to support the "big block of parameters" style. We don't have auto as a keyword, but plausibly we could try to parse |
As you are thinking about new syntax, I would like to suggest |
I do rather like |
If we're volunteering new syntax, what about a slight tweak:
assuming that parsed somehow. To me it makes it clear that it's doing a small code transformation like any macro. I'm sure there are many other uses for macros in that position. |
I would have thought
Really? I don't see it... Since |
See #54114
I'd like it to support
@typeconst x = 3.1; @typeconst x = 3
, as well asx = 2; @typeconst x=3.2
, but this requires figuring out if x already has a type assertion, and I don't know how to do that...