parser + gen-rust: hyphenated use paths, and a module-level var is static mut (Refs #2161) - #2733
Merged
Conversation
Module NAMES have accepted hyphens since the module-declaration parser
was written. `use` paths never did, so
use tritype-base::Trit;
read the import as `tritype` and left `-base::Trit` behind as a
module-level expression statement. That phantom reached gen-verilog as
-base_Trit;
a line the simulator rejects, and no diagnostic mentions it because from
the parser's side nothing went wrong: the `use` parsed, the leftover
parsed, and both were accepted.
`read_hyphenated_ident` factors the loop the module parser already has
and is called at both places a `use` segment is read -- the first one and
each one after `::`. The braced form `use a-b::{X, Y}` works for the same
reason: `full_path` now ends in `::` before the `{`, which is exactly the
precondition the W630 braced-import block already tests.
Measured over 746 tracked specs, master binary vs this one:
phantom `-name;` statements in generated Verilog 28 -> 0
specs emitting one 11 -> 0
specs whose Verilog output changes 11
specs that parse 620 -> 620
t27c tests 1629/6 -> 1629/6
The parse count does not move: 18 specs carry a hyphenated `use`, and the
three that fail `parse` fail on defects behind this one. What this fixes
is the silent half -- eleven specs that parsed, generated, and shipped
invalid Verilog.
FROZEN_HASH resealed in the same commit (M5).
…nsafe (Refs #2161, closes #2731) I filed #2731 saying this needed an owner decision because Rust has no safe mutable global. Re-reading the other three backends settles it: they already agree. gen-verilog lowers a module-level `var` to a `reg`, gen-c to a `static`, Zig to a `var` -- all three mean SHARED mutable state. Of the three Rust candidates, only `static mut` means that; `AtomicU32` changes the API and `thread_local!` changes the semantics to per-thread, which would make Rust the one backend disagreeing about what the source says. So the decision was already in the tree, in the form of what the other three do. #2731 asked a question the repository had answered. pub static mut counter: u32 = 0; pub fn bump() -> u32 { unsafe { counter = (counter + 1); return counter; } } Every access to a `static mut` is unsafe. Wrapping the whole body is the smallest correct answer -- the alternative needs the expression emitter to know the name set at each site. `static_mut_names` is collected in a PRE-PASS, because a function can be emitted before the declaration it reads. Measured over the 43 specs whose Rust output this changes: rustc errors 921 -> 760 specs clean 0 -> 0 The second row is the honest one: not one of these specs compiles yet, because they carry other defects -- Zig builtins leaking into the Rust output chief among them. What this fixes is the declaration and its readers, which were wrong on their own terms. A function that touches no module-level mutable is emitted byte for byte as before -- checked. No regressions: parse 620/746 unchanged, tests 1629 passed / 6 failed unchanged, RATCHET: CLEAN. FROZEN_HASH resealed in the same commit (M5).
Contributor
|
📓 NotebookLM Notebook linked to this PR
This notebook contains session context, decisions, and artifacts for this work. |
Contributor
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Two defects, both about a lowering that three backends agreed on and a fourth did not.
A hyphen is part of the name in a
usepathModule names have accepted hyphens since the module-declaration parser was written.
usepaths never did:read the import as
tritypeand left-base::Tritbehind as a module-level expression statement. That phantom reached gen-verilog as-base_Trit;— a line the simulator rejects, and no diagnostic ever mentioned it, because from the parser's side nothing went wrong: theuseparsed, the leftover parsed, and both were accepted.-name;statements in generated VerilogThe braced form
use a-b::{X, Y}works for the same reason:full_pathnow ends in::before the{, which is exactly the precondition the W630 braced-import block already tests.A module-level
varisstatic mutI filed #2731 saying this needed an owner decision, because Rust has no safe mutable global. Re-reading the other three backends settles it — they already agree:
regstaticvarOf the three Rust candidates only
static mutmeans that.AtomicU32changes the API to load/store;thread_local!changes the semantics to per-thread, which would make Rust the one backend disagreeing about what the source says.The repository had already answered the question I asked it. Closes #2731.
Compiles and runs, printing
1 2— the same answer the C does.static_mut_namesis collected in a pre-pass, because a function can be emitted before the declaration it reads.The second row is the honest one: none of them compiles yet, because they carry other defects — Zig builtins leaking into the Rust output chief among them. What this fixes is the declaration and its readers, which were wrong on their own terms.
A function that touches no module-level mutable is emitted byte for byte as before — checked.
No regressions
parse 620/746 unchanged, tests 1629 passed / 6 failed unchanged, RATCHET: CLEAN.
Refs #2161