Skip to content

CFE does not reject non-const partial instantiations for default parameter values #32912

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

Closed
leafpetersen opened this issue Apr 17, 2018 · 14 comments
Assignees
Labels
front-end-missing-error front-end-requires-constant-evaluation Issue that can't be solved without evaluating compile-time constants legacy-area-front-end Legacy: Use area-dart-model instead. P2 A bug or feature request we're likely to work on
Milestone

Comments

@leafpetersen
Copy link
Member

This is the CFE specific issue for the default parameter issue from #32415 .

Partial instantiations of generic methods are only const objects if their type arguments are const types. The CFE should statically reject the following program, but currently doesn't.

dynamic _defaultCallback<T>(T t) => t;
class C<T> {
  final dynamic Function(T) callback;
  // Should be statically rejected
  void foo([dynamic Function(T) f = _defaultCallback]) {}
  // Should be statically rejected
  const C({this.callback = _defaultCallback});
}

// Should be statically rejected
void bar<T>([dynamic Function(T) f = _defaultCallback]) {}

void main() {
  print(new C<int>().callback);
}
@dgrove
Copy link
Contributor

dgrove commented Jun 15, 2018

With -dev.59, the Dart Analyzer doesn't report any errors for this. /cc @bwilkerson

@kmillikin kmillikin added the P2 A bug or feature request we're likely to work on label Jun 15, 2018
@mit-mit
Copy link
Member

mit-mit commented Jul 9, 2018

@peter-ahe-google to check if this fails in the VM

@peter-ahe-google
Copy link
Contributor

Dart2js is rejecting this program, and so is the VM.

@mit-mit
Copy link
Member

mit-mit commented Jul 9, 2018

Per triage discussion, and above comment, punting to 2.1

@mit-mit mit-mit modified the milestones: Dart2Stable, Dart2.1 Jul 9, 2018
@natebosch
Copy link
Member

and so is the VM.

I see a runtime error. How can I repro the VM statically rejecting this code? Tried at Dart VM version: 2.0.0-dev.67.0 and Dart VM version: 2.0.0-edge.bab152a0a903aeb7d4afc974d5368ce220dd9501

cc @dgrove - per our discussion regarding breaking changes I think either this or #32913 should be in Dart2Stable.

@dgrove
Copy link
Contributor

dgrove commented Jul 9, 2018

FWIW, I see dart2js (which uses the CFE) rejecting this with -dev.67, but I'm not able to see the VM rejecting it statically (I do see a runtime VM error).

@peter-ahe-google
Copy link
Contributor

I could have sworn that I saw the VM report this as a compile-time error, but I must have accidentally looked at the output from dart2js.

@leafpetersen
Copy link
Member Author

Note that dart2js only gives an error on this if the default value is actually used. So the code below is not statically rejected, and gives no errors when run (neither by DDC, nor by dart2js, nor by the VM).

dynamic _defaultCallback<T>(T t) => t;
class C<T> {
  final dynamic Function(T) callback;
  // Should be statically rejected
  void foo([dynamic Function(T) f = _defaultCallback]) {}
  // Should be statically rejected
  const C({this.callback = _defaultCallback});
}

// Should be statically rejected
void bar<T>([dynamic Function(T) f = _defaultCallback]) {}

void main() {
  print(new C<int>(callback: _defaultCallback));
}

This means that fixing this will be a breaking change.

@askeksa-google askeksa-google added the front-end-requires-constant-evaluation Issue that can't be solved without evaluating compile-time constants label Aug 22, 2018
@a-siva
Copy link
Contributor

a-siva commented Oct 11, 2018

Leaf has already reported the behavior on the different implementations in his comment above.

@kmillikin
Copy link

The Dart analyzer does not catch this. The JIT VM fails if the value is used:

Unhandled exception:
type '(T) => dynamic' is not a subtype of type '(int) => dynamic'
#0      main (const.dart:14:22)
#1      _startIsolate.<anonymous closure> (dart:isolate/runtime/libisolate_patch.dart:289:19)
#2      _RawReceivePortImpl._handleMessage (dart:isolate/runtime/libisolate_patch.dart:171:12)

The AOT VM compiler crashes when deserializing a .dill whether or not the value is used:

error: Unbound type parameter found in Library:'const.dart' Class: ::.  Please report this at dartbug.com.

So we're not really catching this but we're not running it either. I don't know how much it would take to fix the VM's JIT and AOT constant evaluators to catch this.

@kmillikin
Copy link

/cc @mkustermann

@mkustermann
Copy link
Member

mkustermann commented Oct 24, 2018

I've made a CL which would fix it for JIT/AOT VM and give better (though still not very good) error messages.

dart-bot pushed a commit that referenced this issue Oct 25, 2018
…ntiation expression

This makes us report the following error

  * in JIT:

  Unhandled exception:
  'test.dart': error: Type arguments must be instantiated in partial instantiation.
  #0      main (...)
  ...

  * in AOT:

  test.dart:7:28: Error: The type '#lib1::C::T' is not a constant, only instantiated types are ...
    const C({this.callback = _defaultCallback});
                           ^
  test.dart:7:17: Context: While analyzing:
    const C({this.callback = _defaultCallback});
                ^
  test.dart:5:37: Error: The type '#lib1::C::T' is not a constant, only instantiated types are ...
    void foo([dynamic Function(T) f = _defaultCallback]) {}
                                    ^
  test.dart:5:33: Context: While analyzing:
    void foo([dynamic Function(T) f = _defaultCallback]) {}
                                ^
  test.dart:11:38: Error: The type '#lib1::bar::T' is not a constant, only instantiated types are ...
  void bar<T>([dynamic Function(T) f = _defaultCallback]) {}
                                     ^
  test.dart:11:34: Context: While analyzing:
  void bar<T>([dynamic Function(T) f = _defaultCallback]) {}



Issue #32912

Change-Id: I05c7019119a50a9cc38939d9cb41aeaa06c853bc
Reviewed-on: https://dart-review.googlesource.com/c/81278
Auto-Submit: Martin Kustermann <kustermann@google.com>
Commit-Queue: Vyacheslav Egorov <vegorov@google.com>
Reviewed-by: Vyacheslav Egorov <vegorov@google.com>
Reviewed-by: Kevin Millikin <kmillikin@google.com>
@kmillikin
Copy link

This is now caught by the VM in both its JIT and AOT compilers. The remaning work on the front end will be done post-Dart 2.1.

@kmillikin kmillikin modified the milestones: Dart2.1, PostDart2.1 Oct 25, 2018
@aadilmaan aadilmaan modified the milestones: Future, D25 Release Jun 4, 2019
@aadilmaan aadilmaan added this to the Future milestone Jun 4, 2019
@askeksa-google
Copy link

Closing because there is no more work to do on this except flipping the flag. This will give us a better overview of remaining issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
front-end-missing-error front-end-requires-constant-evaluation Issue that can't be solved without evaluating compile-time constants legacy-area-front-end Legacy: Use area-dart-model instead. P2 A bug or feature request we're likely to work on
Projects
None yet
Development

No branches or pull requests