Support for generic method syntax in the VM fails too aggressively in checked mode.

[eernstg]

The support for generic method syntax added by the commit 11562f6 in response to #25869 may be too aggressive in its treatment of checked mode checks. In particular, the following program encounters a runtime error:

T identity<T>(T t) => t;

main() {
  print(identity<int>(42));
}

If this program is stored in foo.dart and dart --checked --generic-method-syntax foo.dart is executed (where dart --version prints "...1.20.0-edge.79b682b885d770e4d073c573dff0b49fdee33d77..."), it fails with the following message (absolute path shortened to ... for readability):

Unhandled exception:
'file:///.../foo.dart': malformed type: line 1 pos 15: function type parameter 'T' not yet supported
T identity<T>(T t) => t;
              ^

The approach taken in dart2js with --generic-method-syntax is to omit passing actual type arguments, and then to consider formal function type parameters as dynamic when used as a type annotation or as a type argument (to an instance creation or a type, e.g., in a type annotation), and raise a runtime error on is and as checks. This allows developers to run programs using generic functions/methods in checked mode, getting all the checks that they would otherwise get, and just "turning off" the checks directly involving function type parameters. I believe that a similar approach would be useful for the vm, and also that it would be useful for the vm and dart2js to agree on the behavior.

[crelier]

It would be a rather big change in the VM to postpone the mapping of a function type parameter to a malformed type or to dynamic until the type is actually used and how it is used. It is why we map it now immediately to a malformed type, which happens to be mapped to dynamic in certain scenarios specified by the spec and that have nothing to do with generic methods.
Now, we only resolve an identifier to a function type parameter at two locations in the parser, one where we resolve a type and one where we process a primary expression. I think that if we only mark the second one as malformed, it may achieve the temporary result that you expect.

I am not currently able to help much (too long to explain, check my calendar), but here is a patch that should achieve what you want, at least for your example.

@a-siva, if you do not mind trying it (you will have to adjust language test expectations) and committing it if it works, that would be awesome. Thanks.

Otherwise, I will have to look at it when I am back.

diff --git a/runtime/vm/parser.cc b/runtime/vm/parser.cc
index def0cfd..8149bb3 100644
--- a/runtime/vm/parser.cc
+++ b/runtime/vm/parser.cc
@@ -12383,15 +12383,8 @@ void Parser::ResolveType(ClassFinalizer::FinalizationKind finalization,
NULL));
if (!type_parameter.IsNull()) {
// TODO(regis): Check for absence of type arguments.

•      // For now, return as malformed type.
  

•      Type& malformed_type = Type::ZoneHandle(Z);
  

•      malformed_type = ClassFinalizer::NewFinalizedMalformedType(
  

•          Error::Handle(Z),  // No previous error.
  

•          script_,
  

•          type->token_pos(),
  

•          "function type parameter '%s' not yet supported",
  

•          String::Handle(Z, type_parameter.name()).ToCString());
  

•      *type = malformed_type.raw();
  

•      // For now, resolve the function type parameter to dynamic.
  

•      *type = Type::DynamicType();
     return;
   }
  

  }

[eernstg]

dart2js forces every function type parameter to be considered to have the value dynamic, and code generation for is T and as T where T is a function type parameter unconditionally raises a runtime error. I don't know if a similar approach would fit in the vm.

[crelier]

As I mentioned, this would require either postponing the mapping of T to dynamic until T is used (rather a big change) or somehow marking the resolved dynamic type from T as unusable in is and as tests (also a non trivial change). I was hoping that my patch would get us there, at least most of the way. If not, I am afraid this will have to wait until I am back.

[leafpetersen]

I'm thinking that this #27460 may be a higher priority issue to follow up on.

[crelier]

I agree. And this is much easier to handle this way.
I sent a cl to address #27460 in the VM.

[floitschG]

Not even arguing that this is the wrong solution, but isn't Erik's approach already fully implemented in the VM when using deferred libraries?

If you replaced x.A or x.B with a generic type argument you would have Erik's behavior:

import 'foo.dart' deferred as x;

x.B foo(x.B tt) {
  return tt;
}

main() {
  x.A toto = foo(null);
  print(toto as x.A);
}

[crelier]

Deferred types are treated as malformed types, in all contexts.
This line added to your example
print(toto is x.A);
throws in the same way as
print(toto as x.A);

Yes, this is how we implemented function type arguments earlier. Now, we map to dynamic in all contexts. Deciding on malformed or dynamic depending on the usage complicates the implementation. That was my point.

[floitschG]

I missed the as checks.
It looks like dynamic it is...

[leafpetersen]

Closing this in favor of #27460