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

[Analyzer] Expression.staticParameterElement returns synthetic elements for generic functions #54669

Closed
abitofevrything opened this issue Jan 18, 2024 · 14 comments
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on

Comments

@abitofevrything
Copy link
Contributor

abitofevrything commented Jan 18, 2024

Simiarly to #48500, Expression.staticParameterElement will return a synthetic ParameterMember when the expression is an argument to a generic function or method. There doesn't seem to be an obvious way to find the actual ParameterElement in the element tree from the synthetic ParameterMember (ParameterMember.declaration is a ParameterElement, but is also synthetic and has no parent).

This applies to:

  • Top level functions
  • Static methods
  • Instance methods
class Test {
  Test.constructor(arg);
  void instanceGeneric<T>(arg) {}
  static void staticGeneric<T>(arg) {}
}

void generic<T>(arg) {}

void test(Test instance) {
  // IntegerLiteral.staticParameterElement is non synthetic.
  Test.constructor(0);

  // IntegerLiteral.staticParameterElement is synthetic.
  instance.instanceGeneric(0);
  Test.staticGeneric(0);
  generic(0);
}

Dart version: 3.4.0-24.0.dev
Analyzer version: 6.3.0

@parlough parlough added the area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. label Jan 18, 2024
@pq
Copy link
Member

pq commented Jan 18, 2024

@bwilkerson

@pq pq added the P2 A bug or feature request we're likely to work on label Jan 18, 2024
@bwilkerson
Copy link
Member

While I can see how it seems odd that you're getting a synthetic element, I'm not sure what problem that's causing you (so I don't know whether there's a work-around that I can point you to). Is there information you need that the synthetic member doesn't have, or is there some reason why you think you need the non-synthetic element?

@abitofevrything
Copy link
Contributor Author

abitofevrything commented Jan 19, 2024

My use case is getting metadata for a TypeParameterElement from a function argument typed as such, but I access the corresponding ParameterElement using Expression.staticParameterElement.

e.g I want to get the @foo on T from IntegerLiteral.staticParameterElement:

const foo = Object();

void test<@foo T>(T arg) { ... }

void main() {
  test(1); // <== Here
}

The issue arises because the (synthetic) element returned by staticParameterElement also references a synthetic TypeParameterElement (when accessed with (expression.staticParameterElement.type as TypeParameterType).element), which doesn't have the correct metadata (it's empty). The bound type and name are correct.

@abitofevrything
Copy link
Contributor Author

The synthetic elements appear to come from invocation_inferrer.dart:188:

// ...
} else {
  // getFreshTypeParameters creates synthetic copies of the type parameter elements, but does not copy over metadata.
  rawType = getFreshTypeParameters(rawType.typeFormals)
      // applyToFunctionType creates synthetic parameter elements
      // (via ParameterElementExtensions.copyWith) with the new synthetic type parameters (even if the parameter does
      // not use the type parameter)
      .applyToFunctionType(rawType);

// ...

The synthetic ParameterElements are then wrapped in ParameterMembers later on (line 238) when the function type is instanciated with the inferred type arguments.

@abitofevrything
Copy link
Contributor Author

It'd also be helpful for ParsedLibraryResult.getElementDeclaration to work on these ParameterElements for my usecase, which requires them to be non synthetic.

@bwilkerson
Copy link
Member

Thanks for the use cases. Unfortunately, I can't think of any easy, clean work-arounds that are currently available.

Do the synthetic parameter elements have enough information to be able to get back to the ExecutableElement from which they came (the function test in the example above)? If so it might be possible to figure out which of the original function's parameters corresponds to the synthetic parameter be comparing the names or offsets. It would be an ugly way to do this, but it might get you unblocked. It could easily fail in invalid code (if there are two parameters with the same name); I don't know whether that would be a concern.

@abitofevrything
Copy link
Contributor Author

I don't think so. ParameterElementExtensions.copyWith seems to throw away any reference to the original element (including its offset). It only preserves the parameter name, type, kind and variance, none of which are useful for locating the ExecutableElement. Same deal for getFreshTypeParameters, which only preserves the name, variance and bound for the TypeParameterElements.

Interestingly the synthetic type parameters are not marked as synthetic, even though their offset is set to -1.

@abitofevrything
Copy link
Contributor Author

I'll try working on a fix, but I'm not sure if it will fit in with the code style of the rest of the codebase.

@bwilkerson
Copy link
Member

Thanks! It might be beneficial (as in save you some time and effort) to start with a short design proposal that we can look at to see whether the direction you're thinking of would be acceptable.

@scheglov For additional pointers.

@abitofevrything
Copy link
Contributor Author

It turns out FullInvocationInferrer.resolveInvocation only really uses the FunctionType with the synthetic type parameter elements to infer the type for the deferredFunctionLiterals. If instead of instanciating the function type with the synthetic type parameters with the inferred type arguments, we instanciate the original function type (with non-synthetic parameter elements) that was passed to resolveInvocation, everything behaves as expected.

Figuring that out was relatively quick. Going through and updating all the tests for other behaviour that were written expecting the parameter members to be synthetic is taking longer :)

The change itself is only 3 lines, so I'll just upload it directly. If it doesn't work, I won't actually have wasted that much time.

@scheglov
Copy link
Contributor

scheglov commented Feb 7, 2024

It was quite long time since I touched the inference code, so I probably know as much about it as anyone else.

One complication with using non-synthetic ParameterElement that I know about is summaries. We consider FunctionTypes as "values", detached from the original location, e.g. a method that declared them, and create new synthetic FunctionType instances as we do type parameter substitutions, LUB, etc - any type operations.

I for long time consider using staticParameterElement as somewhat cumbersome, and that we probably should use some other mechanism to get from an argument to the ParameterElement of the invoked method. When this is a MethodInvocaiton like target.foo(a) and foo is a method. Or nothing, if foo is a FunctionType typed field.

@abitofevrything
Copy link
Contributor Author

@abitofevrything
Copy link
Contributor Author

I did notice summaries might be an issue (anywhere that uses getFreshTypeParameters is, really) but I haven't worked with them at all so I wouldn't know where this could be an issue.

@abitofevrything
Copy link
Contributor Author

Fixed in 7ad5a57

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on
Projects
None yet
Development

No branches or pull requests

5 participants