Skip to content

Make AST nodes immutable #1067

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

Closed
whitequark opened this issue Jan 30, 2024 · 3 comments · Fixed by #1165
Closed

Make AST nodes immutable #1067

whitequark opened this issue Jan 30, 2024 · 3 comments · Fixed by #1165

Comments

@whitequark
Copy link
Member

whitequark commented Jan 30, 2024

Right now it's actually possible to do things like "set sig.width on a Signal" or "modify c.parts on a Cat". This has never been explicitly advertised as something that is supported and is arguably a misuse of the API. Amaranth internally treats AST nodes as immutable; all fragment transformers create new ones, and it doesn't mutate them elsewhere. Any mutation would happen only in downstream code (there's one instance deep in the FSM code generation, #1066).

Now that I'm working on reference documentation I'd like to either state that Values are immutable and make them so, or else document them as mutable. This could potentially break some downstream code though, so I'm nominating this for discussion.

If we decide that the nodes should be mutable, then setters must be introduced that typecheck the assignments.

@whitequark
Copy link
Member Author

We have discussed this change on the 2024-02-12 weekly meeting. The decision was to make all Value hierarchy have immutable attributes.

This will only happen after we find a solution to #1066 though.

@whitequark whitequark changed the title Should AST nodes be immutable? Make AST nodes immutable Feb 13, 2024
@whitequark
Copy link
Member Author

This is now unblocked since #1066 is solved.

wanda-phi added a commit to wanda-phi/amaranth that referenced this issue Feb 28, 2024
@whitequark
Copy link
Member Author

whitequark commented Feb 29, 2024

This is completed by #1165, with the caveats that Signal.name and Value.src_loc/Statement.src_loc continue to be mutable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

1 participant