[red-knot] Add symbol and definition for parameters#12862
Conversation
5f1db52 to
706526d
Compare
|
706526d to
34b786b
Compare
CodSpeed Performance ReportMerging #12862 will degrade performances by 17.64%Comparing Summary
Benchmarks breakdown
|
|
I need to look at the regression. |
AlexWaygood
left a comment
There was a problem hiding this comment.
Mostly this looks great to me. I have a style nit (feel free to ignore if you disagree!) and a question:
| { | ||
| self.visit_expr(default); | ||
| } | ||
| self.visit_parameters(parameters); |
There was a problem hiding this comment.
This visit_parameters() call here is confusing me.
- Why do we call
self.visit_parameters()for aLambdaexpression, but not for aFunctionDefstatement? - Is it correct for the
visit_parameters()call to take place before theself.push_scope()call? Exactly which scope annotations should be evaluated in is a complicated question because it's affected byfrom __future__ import annotationsor whether it's a stub file, so this might be correct. But if it is correct, then why can't the default values just be visited as part of thevisit_parameters()call? - If we keep visiting the default values outside the
visit_parameters()call, what wouldvisit_parameters()do for a lambda function anyway? Lambda functions can't have annotations on their parameters, so for lambdas the only things we need to analyse are the binding the parameter creates and the parameter's default value (if it has one). Both the binding and the default value seem to be taken care of outside thevisit_parameters()call
There was a problem hiding this comment.
I think you are right that this call could be removed for lambda, because we already visited the defaults and won't visit them again here, and there can't be annotations, so there is nothing else left to visit. However, it still seems marginally clearer and more robust to me to have the call here. In a hypothetical future where lambdas gain argument annotations (or something else waves hands wildly is added to parameters), we would just naturally do the right thing, instead of subtly missing a visit to part of the AST.
I don't think from __future__ import annotations or stub-file-ness impact which scope annotations should be evaluated in, just whether they are evaluated immediately or deferred. If a lambda could have annotations, the outer scope would be the correct one to evaluate them in (since lambdas can't have a type params scope.) So I think this call is located in the correct place.
why can't the default values just be visited as part of the visit_parameters() call?
For lambdas, they could. I think an alternate approach here would be to add visit_parameters_without_defaults and use that in the FunctionDef case (rather than overriding visit_parameters), and then remove the special code to visit defaults from the lambda case and just rely on visit_parameters here. I don't have a strong feeling that this is better, though -- the current approach more closely mirrors how type inference handles it. I do think it's nice to keep a parallel between the handling of lambda and FunctionDef, which the current code in the PR does.
For FunctionDef the call looks like builder.visit_parameters(...), and is inside the type params scope (if any), which is the correct place for it.
There was a problem hiding this comment.
I think @carljm answered to most of @AlexWaygood questions (thanks Carl).
- If we keep visiting the default values outside the
visit_parameters()call, what wouldvisit_parameters()do for a lambda function anyway? Lambda functions can't have annotations on their parameters, so for lambdas the only things we need to analyse are the binding the parameter creates and the parameter's default value (if it has one). Both the binding and the default value seem to be taken care of outside thevisit_parameters()call
Yes, technically there's no benefit of calling visit_parameters for lambdas here. I think it does make sense to keep it for consistency and in case the Parameters node is changed in the future by either us or CPython this will provide us some kind of failure (I think).
For lambdas, they could. I think an alternate approach here would be to add
visit_parameters_without_defaultsand use that in theFunctionDefcase (rather than overridingvisit_parameters), and then remove the special code to visit defaults from the lambda case and just rely onvisit_parametershere. I don't have a strong feeling that this is better, though -- the current approach more closely mirrors how type inference handles it. I do think it's nice to keep a parallel between the handling of lambda and FunctionDef, which the current code in the PR does.
I like the idea although I think I'd prefer consistency here.
It might be just because there's now lots more symbols in the benchmark that we need to try to infer types for |
carljm
left a comment
There was a problem hiding this comment.
Looks excellent, thank you!
| { | ||
| self.visit_expr(default); | ||
| } | ||
| self.visit_parameters(parameters); |
There was a problem hiding this comment.
I think you are right that this call could be removed for lambda, because we already visited the defaults and won't visit them again here, and there can't be annotations, so there is nothing else left to visit. However, it still seems marginally clearer and more robust to me to have the call here. In a hypothetical future where lambdas gain argument annotations (or something else waves hands wildly is added to parameters), we would just naturally do the right thing, instead of subtly missing a visit to part of the AST.
I don't think from __future__ import annotations or stub-file-ness impact which scope annotations should be evaluated in, just whether they are evaluated immediately or deferred. If a lambda could have annotations, the outer scope would be the correct one to evaluate them in (since lambdas can't have a type params scope.) So I think this call is located in the correct place.
why can't the default values just be visited as part of the visit_parameters() call?
For lambdas, they could. I think an alternate approach here would be to add visit_parameters_without_defaults and use that in the FunctionDef case (rather than overriding visit_parameters), and then remove the special code to visit defaults from the lambda case and just rely on visit_parameters here. I don't have a strong feeling that this is better, though -- the current approach more closely mirrors how type inference handles it. I do think it's nice to keep a parallel between the handling of lambda and FunctionDef, which the current code in the PR does.
For FunctionDef the call looks like builder.visit_parameters(...), and is inside the type params scope (if any), which is the correct place for it.
34b786b to
c47b7f6
Compare
Yeah, this is correct. The definition counts are doubled because we now have definition for each parameters in all the functions. On On And, the flamegraph shows that the time spent in Acknowledging the regression based on the above findings. |
Summary
This PR adds support for adding symbols and definitions for function and lambda parameters to the semantic index.
Notes
Type Inference
There are two definitions
ParameterandParameterWithDefaultand their respective*_definitionmethods on the type inference builder. These methods are preferred and are re-used when checking from a different region.Test Plan
Add test case for validating that the parameters are defined in the function / lambda scope.
Benchmark update
Validated the difference in diagnostics for benchmark code between
mainand this branch. All of them are either directly or indirectly referencing one of the function parameters. The diff is below:Details