Move type alias attribute validation to parser #1916
Merged
Conversation
This file contains 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
bbannier
previously approved these changes
Nov 7, 2024
evantypanski
force-pushed
the
topic/etyp/cleanup-parsing-attributes-2
branch
from
4e9b286 to
83131ca
Compare
Closes #1898 This was originally in a validator, but there's some sense that this is a parsing issue since attributes should not be allowed at all on most constructs of that form. Note that a lot of the cleanup here was meant to span removing the `opt_attributes` field in `type_decl` altogether, but the fallout from that is pretty large. It ended up being less cleanup and more rearchitecting. This way still avoids the hacky test for type aliases in the validator while not changing the AST structure.
bbannier
force-pushed
the
topic/etyp/cleanup-parsing-attributes-2
branch
from
83131ca to
0744bab
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #1898
This just moves the logic from #1890 to an allow list of types that can have attributes in the parser so that we avoid a hacky check. I don't really like checks in the parser like this very much, but since this fixes a weird validation I think it's ok.
The rest of this is just on what I chose not to do, which may be interesting depending what you think. That's all on the
topic/etyp/cleanup-parsing-attributesbranch now.My initial impression was that attributes should apply to types, then remove
opt_attributesfrom thetype_decl. That technically seems possible, but I'd hardly call it a cleanup. It essentially requires duplicating the attributes logic into manyUnqualifiedTypeimplementations (struct, enum, etc. but also likeexception).This goes into hilti world, so now the hilti parser needs changed, and once you get there all bets are off. You can use the
&on-heapattribute on a lot of things, which is a declaration attribute. That's because what that means for a type is just that it becomes avalue_ref. So now we think about these attributes on declarations as type attributes, which just doesn't intuitively make sense. I guess you could try to have both, but since there isn't a great distinction from syntax alone, it's hard.There's a fair bit more, but I think the attribute changes should try to stay minimal unless we're going to really rework them, which I kind of advocated for in the issue this is closing... but that's a big change :)
As an added point: The branch I did that on had some positive consequences - tests like
unsupported_attribute.spicyflag an error on an attribute, but the range given does not even encompass the attribute. I added a continuation of #1899 for unit'ssetAttributesfor that case just as an added bonus.