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
Fix body locations for def nodes that have default args #10619
Conversation
Currently it is reported that the actual body of the method begins at the first "assignment" to a default arg, if there is one: \/here def foo(x, y=5) test /\not here end I think that's a bit arbitrary and won't be helpful for whatever tooling may rely on this, so I change it to be as shown "not here" on the diagram. This also fixes the body location for `def self.new` generated from `def initialize`.
Specs that I'm adding were previously failing:
|
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.
Great!
spec/compiler/normalize/def_spec.cr
Outdated
describe "gives correct body location with" do | ||
{"default arg" => "def testing(foo = 5)", | ||
"default arg with restriction" => "def testing(foo : Int = 5)", | ||
"splat arg" => "def testing(*foo : Array)", | ||
"block instance var arg" => "def testing(&@foo : ->)", | ||
}.each do |(suf, definition)| | ||
describe suf + "," do | ||
{"with body" => "zzz = 7\n", "without body" => ""}.each do |(suf, body)| | ||
it suf do | ||
a_def = parse("#{definition}\n#{body}end").as(Def) | ||
actual = a_def.expand_default_arguments(Program.new, 1) | ||
|
||
actual.location.should eq Location.new("", line_number: 1, column_number: 1) | ||
actual.body.location.should eq Location.new("", line_number: 2, column_number: 1) | ||
end | ||
end | ||
end | ||
end | ||
end |
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.
As a general consideration, I don't like specs in iterations because they seem unnecessarily hard to understand. Especially with nested iterations.
My preferred style would be a spec helper called with explicit source code of each combination. That also offers much flexibility and it would be easy to add specs for multi-line parameter lists for example.
I'm happy to accept this as is though.
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 don't mind, but in this particular case I don't see a good way to write this with a helper, particularly due to the nested iterations.
I think I will not do this change.
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 would be my proposal: straight-shoota@5da5886
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.
Hmm some things there that make me like that one less:
- The
with body
context comes in the wrong order (precedence). - No possibility of auto-indenting the different cases.
- It is indeed a lot more code, and it's not like the helper will be reused for different tests.
For now I tweaked my version, maybe it looks a little better
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.
Currently it is reported that the actual body of the method begins at the first "assignment" to a default arg, if there is one:
I think that's a bit arbitrary and won't be helpful for whatever tooling may rely on this, so I change it to be shown at the point of the "not here" example on the diagram.
This also fixes the body location for
def self.new
generated fromdef initialize
.