-
Notifications
You must be signed in to change notification settings - Fork 148
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
Type every node field and mark on-error-only types explicitly #3022
Conversation
eregon
commented
Aug 28, 2024
- For Loader.java, do not deserialize the AST if there are errors, so then Java nodes only have non-error types for fields.
options[:kind] = kinds | ||
else | ||
if field.fetch("type").include?("node") | ||
raise "Missing kind in config.yml for field #{@name}##{options.fetch(:name)}" |
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.
This will ensure every node field going forward will have a type
d7247c5
to
3432aa1
Compare
Rust is failing, and the reason is it doesn't reuse the logic in template.rb but duplicates some of it. I guess I will have to give up on |
90a036f
to
1792959
Compare
templates/template.rb
Outdated
when "pattern expression" | ||
# TODO: can we be more precise, is the list small enough? | ||
"Node" |
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.
FWIW this is the full list I got from running the test suite:
AlternationPatternNode
ArrayPatternNode
CapturePatternNode
FindPatternNode
HashPatternNode
PinnedExpressionNode
PinnedVariableNode
LocalVariableTargetNode
ImplicitRestNode
SplatNode
IfNode
UnlessNode
SymbolNode
InterpolatedSymbolNode
TrueNode
FalseNode
NilNode
IntegerNode
FloatNode
RangeNode
StringNode
InterpolatedStringNode
XStringNode
RegularExpressionNode
InterpolatedRegularExpressionNode
SourceFileNode
SourceLineNode
SourceEncodingNode
ParenthesesNode
ArrayNode
RationalNode
ImaginaryNode
SelfNode
ConstantReadNode
ConstantPathNode
LambdaNode
MissingNode
A 37-wide union type is probably not very practical so I'll leave it as Node
here and remove the TODO
27a2b55
to
e769282
Compare
@@ -1,5 +1,5 @@ | |||
#!/usr/bin/env ruby | |||
# typed: false | |||
# typed: ignore |
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.
Sorbet would complain about NodeKindField#initialize
.
I tried to declare & pass name and comment kwargs explicitly, but it still wasn't happy and only suggested using T.unsafe
.
It seems weird that Sorbet would fail for something so simple, but anyway it seems like a bug it even complains about this in a typed: false
file.
end.compact | ||
if kinds.size == 1 | ||
kinds = kinds.first | ||
kinds = nil if kinds == "Node" |
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.
I initially tried for the kind to just be "Node"
if there is nothing more precise but that causes a bunch of subtle bugs in templates, so I make it nil
in that case like before. It's necessary to treat Node
specially in several cases and comparing against a String doesn't seem any better.
This should be green now and ready for review. |
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.
Yeah I think I'm good with the general direction of this. I need to take a minute to do a full review (I'm not 100% sure all of the places where you marked non-void are actually non-void, they may allow void expressions like break
). Only one comment to change at the moment.
e769282
to
27ca419
Compare
FWIW I checked most |
@kddnewton Could you merge this? I could also re-check every use of |
I made a pass over all
The way I checked is looking at transitive callers of
And for the rest I tried with examples with |
526697f
to
ee327b4
Compare
|
ee327b4
to
ae78e3c
Compare
* For Loader.java, do not deserialize the AST if there are errors, so then Java nodes only have non-error types for fields.
CI is green now after rebase. |
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.
Nice work, this seems like it would have taken a lot of effort. I think the typing changes will really help.