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

Type every node field and mark on-error-only types explicitly #3022

Merged
merged 1 commit into from
Sep 25, 2024

Conversation

eregon
Copy link
Member

@eregon 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.

@eregon eregon requested a review from kddnewton August 28, 2024 23:12
config.yml Outdated Show resolved Hide resolved
config.yml Outdated Show resolved Hide resolved
options[:kind] = kinds
else
if field.fetch("type").include?("node")
raise "Missing kind in config.yml for field #{@name}##{options.fetch(:name)}"
Copy link
Member Author

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

@eregon eregon force-pushed the type-everything branch 2 times, most recently from d7247c5 to 3432aa1 Compare August 28, 2024 23:50
@eregon
Copy link
Member Author

eregon commented Aug 28, 2024

Rust is failing, and the reason is it doesn't reuse the logic in template.rb but duplicates some of it.
That's rather unfortunate, is there a reason it can't be generated by template.rb like for other languages?

I guess I will have to give up on - on error: SymbolNode (which is a nested Hash) as Rust is much stricter with types (so maybe use - on error SymbolNode or so).

@eregon eregon force-pushed the type-everything branch 9 times, most recently from 90a036f to 1792959 Compare August 29, 2024 21:34
Comment on lines 429 to 431
when "pattern expression"
# TODO: can we be more precise, is the list small enough?
"Node"
Copy link
Member Author

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

@eregon eregon force-pushed the type-everything branch 2 times, most recently from 27a2b55 to e769282 Compare August 29, 2024 22:10
@@ -1,5 +1,5 @@
#!/usr/bin/env ruby
# typed: false
# typed: ignore
Copy link
Member Author

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"
Copy link
Member Author

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.

@eregon
Copy link
Member Author

eregon commented Aug 29, 2024

This should be green now and ready for review.
I had to learn a bit of Rust but it wasn't too bad (well, except the crazy String types complications).

Copy link
Collaborator

@kddnewton kddnewton left a 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.

config.yml Outdated Show resolved Hide resolved
@eregon
Copy link
Member Author

eregon commented Sep 3, 2024

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.

FWIW non-void expression is typed as Node so whether it's non-void expression or more it doesn't matter in terms of the given types, it's just for documentation purposes in this file.

I checked most non-void expression I added, except when the field was called value where I just assumed that should be non-void expression.

@eregon
Copy link
Member Author

eregon commented Sep 12, 2024

@kddnewton Could you merge this?
I'm worried it's going to get conflicts soon otherwise.
As said above even if a non-void expression should be Node in the config there is no functional issue as they both become Node in types.

I could also re-check every use of non-void expression if you want and it doesn't take too long (based on the parser rules/grammar or based on trying code example with e.g. break). Let me know.

@eregon
Copy link
Member Author

eregon commented Sep 12, 2024

I made a pass over all kind: non-void expression.
Almost all of them are correct, except for the following things:

  • First, even though e.g. foo(return) is a SyntaxError, Prism parses it to CallNode(ArgumentsNode(ReturnNode)) + record errors, so it's possible to have a void expression as a node in the AST returned by Prism (when there are errors). So kind: non-void expression means non-void expression on success, any node on error.
  • AndNode which was already documented to have left and right as non-void expression is not quite as simple. Left is always non-void expression, that's fine. But right has no such requirement, for example a and return is valid. OTOH a and BEGIN{} is invalid. So I think we should change AndNode & OrNode right to be Node and not non-void expression, since some void expressions are allowed there.
  • RescueModifierNode it seems both the expression and rescue_expression can be void expressions: loop { break rescue a } is valid and a rescue return is valid but a rescue BEGIN{} is not.
    I'll change these 3 field kinds to Node.

The way I checked is looking at transitive callers of pm_check_value_expression in prism.c to find the checks.
So that's

pm_check_value_expression
pm_assert_value_expression
parse_value_expression
parse_starred_expression
parse_predicate
parse_assignment_value -> all or/and/operator write nodes

And for the rest I tried with examples with bin/parse.

@eregon eregon force-pushed the type-everything branch 2 times, most recently from 526697f to ee327b4 Compare September 12, 2024 20:39
@eregon eregon requested a review from kddnewton September 12, 2024 20:40
@eregon
Copy link
Member Author

eregon commented Sep 12, 2024

CRuby bindings / test-all has some errors, which I guess means some types need to be changed in prism_compile.c.

* For Loader.java, do not deserialize the AST if there are errors, so then Java nodes only have non-error types for fields.
@eregon
Copy link
Member Author

eregon commented Sep 14, 2024

CI is green now after rebase.

Copy link
Collaborator

@kddnewton kddnewton left a 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.

@kddnewton kddnewton merged commit 92ad483 into ruby:main Sep 25, 2024
55 checks passed
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.

2 participants