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

AstGen: add error for redundant comptime var in comptime scope #18242

Merged
merged 5 commits into from
Jan 10, 2024

Conversation

wrongnull
Copy link
Contributor

Inspired from #18227

@rohlem
Copy link
Contributor

rohlem commented Dec 10, 2023

I thought I remembered a proposal-like discussion somewhere to require comptime var when the type of the variable is comptime-only, but now I can't find it anymore.
Maybe someone else remembers/finds what I mean, I don't remember the full context / motivations behind it anymore either.

My own two cents of why I'm not too fond of this change:

At comptime, variables have the attribute that you can safely return a *const to them from the current scope (see #7396).
This can be used as a sort of compile-time memory allocation.
By marking variables I use this way as comptime var, I am safe from forgetting about / overlooking this should I ever want to convert/enable the code to run at run-time.

Workaround I thought of while writing this comment:
I guess I can explicitly write if (!@inComptime()) @compileError("x is a comptime allocation"); into the code after every such variable x.
It's a bit more to think about and remember, and imo a little less readable, but not bad enough to make a fuss over I suppose.

@mlugg
Copy link
Member

mlugg commented Dec 10, 2023

This is only for cases where comptime is known from syntax alone, such as comptime var in a comptime block. This should be fairly obvious to change; but in general, you can't just expect code to work when you delete comptime keywords.

With regard to the specific use case you mentioned: doing this with comptime var is supposed to be illegal, and will become a compile error in future. You must use comptime-known consts to get a runtime-valid constant pointer. Use const x = comptime ... if you really want to be sure the thing is comptime-known, but usually it's obvious.

@rohlem
Copy link
Contributor

rohlem commented Dec 10, 2023

in general, you can't just expect code to work when you delete comptime keywords.

From my understanding, Zig's explicit goal with comptime semantics is to allow using the same code at compile-time and at run-time
(and to that end, for the code to behave the same in both scenarios).
I realize that we're not 100% there yet, but I personally regard any difference in behavior between run-time and comptime as a usability bug.
We might not get rid of all of them before 1.0, but in my eyes it's worth pointing them out and trying to reduce them.

With regard to the specific use case you mentioned: doing this with comptime var is supposed to be illegal [...] .
You must use comptime-known consts to get a runtime-valid constant pointer.

That conflicts with my current understanding - I cannot find any basis for it in #7396, and don't remember it from any other issues.
Not to sound like I don't believe you (I know and appreciate your work as part of the core team!), I just hadn't read of that distinction before.

Your suggestion of explicitly writing const result = comptime mutable_buffer; and then returning &result indeed seems like a readability improvement,
and I would be in favor of doing that. However even that comptime keyword is disallowed by compile error when already in a comptime block,
so I don't think that works as an in-code reminder/assertion for that scenario either.
I suppose I'll have to stick to the if-@compileError-assertion I mentioned above after this is merged.

EDIT: Rethinking it in light of some comptime hacking work I'm doing right now,
the requirement to copy the data from a comptime var to a comptime-known const
makes working with pointers within comptime memory much more tiresome.
I also think it completely precludes constructing self-referential data structures at comptime
(not that I'm personally a fan of them or anything, but that was a major motivation behind RLS, so someone must have been considering them).
I'd much appreciate some more rationale / discussion surrounding this limitation
(in a separate issue, or maybe a link to where that discussion already took place previously).

@matu3ba
Copy link
Contributor

matu3ba commented Dec 10, 2023

I realize that we're not 100% there yet, but I personally regard any difference in behavior between run-time and comptime as a usability bug.

  1. This is unfeasible to solve in general due to 1.1 Zig not enforcing input of accuracy constraints and the 1.2 input number being also approximated for runtime-known code vs comptime known (for practicability). As example, the set of decimal numbers in integer range with precision 0.1 can not be accurately represented in binary floats with different semantics for comptime and runtime code.
  2. My gut feeling tells me that the only way without forcing the programmer to type more would be to extend the type system to d32, d64, d128 and for arbitrary precision floats as well as arbprec for comptime-only types and default to d32, but this would again break use cases outside of decimal floats and penalize binary float intended applications.
  3. I have not seen yet a good proposal starting with an outline of the problem and the possible solution space. Fixed-point number support? #1974 and Proposal: decimal floating-point types #4221 have barely details, tradeoffs etc. If you're interested in a discussions etc, then please write me on discord.

@rohlem
Copy link
Contributor

rohlem commented Dec 10, 2023

@matu3ba While number precision wasn't mentioned in this thread yet, it's certainly also an interesting topic.
That said, I don't see a fundamental reason why comptime couldn't "emulate the target architecture" in this regard (that's exactly the text currently stated on ziglang.org, lol).
But as many architectures' specifications allow different floating point precisions between cpu models, of course there won't be a single correct answer on what to do.

Anyway, I don't see a connection between floating point imprecision, and lifetime/memory semantics in relation to language syntax.
In my eyes reducing footguns, such as by making comptime behavior closer to run-time behavior, in any area is an improvement.
I don't see floats being annoying as a reason for giving up on such efforts.

@wrongnull
Copy link
Contributor Author

wrongnull commented Dec 11, 2023

In addition to this, I think that @call with .compile_time modifier should cause the same compilation error if called in comptime scope (this could probably be handled in Sema.analyzeCall). So far I'm not sure if I'm right.

@matu3ba
Copy link
Contributor

matu3ba commented Dec 11, 2023

Anyway, I don't see a connection between floating point imprecision, and lifetime/memory semantics in relation to language syntax.

During compilation time, you want portable behavior which also includes at least the option to have portability across floating point operations. Portability across floating point operations mandates arbitrary precision numbers, which require allocations at compilation time to handle etc.

During runtime mostly runtime performance is much more important.

How and if to make both work with the same code paths at scope/fn level remains unclear/not well motivated yet. The comptime block in the langref does not clarify semantics at all.

@andrewrk andrewrk merged commit 4a1a5ee into ziglang:master Jan 10, 2024
10 checks passed
@andrewrk
Copy link
Member

Nice, thank you.

@wrongnull wrongnull deleted the comptime-var branch January 10, 2024 10:39
linusg added a commit to linusg/parser-toolkit that referenced this pull request Jan 10, 2024
ikskuh pushed a commit to ikskuh/parser-toolkit that referenced this pull request Jan 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants