Skip to content

Small relaxation of const constructor error #46758

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
eernstg opened this issue Jul 29, 2021 · 8 comments
Closed

Small relaxation of const constructor error #46758

eernstg opened this issue Jul 29, 2021 · 8 comments
Labels
area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...). type-question A question about expected behavior or functionality

Comments

@eernstg
Copy link
Member

eernstg commented Jul 29, 2021

This issue describes a small, potential relaxation of a compile-time error. It may seem like a small bug fix, but we wish to ensure that the implementation effort isn't large, either, and check that the change does not have unexpected implications.

The relaxation is that a class can have a constant factory constructor and a late final instance variable, which used to be a compile-time error.

Cf. issue dart-lang/language#1763 and PR dart-lang/language#1764, and the following program:

class Base {
  Base();
  const factory Base.empty() = _Empty;
  late final int property;
}

class _Empty implements Base {
  const _Empty();
  int get property => 2;
  set property(_) => throw "Property cannot be initialized";
}

void main() {}

Class Base is an error according to the specification, and it is reported as a compile-time error by the analyzer (from 4fc912c):

ERROR|COMPILE_TIME_ERROR|LATE_FINAL_FIELD_WITH_CONST_CONSTRUCTOR|/usr/local/google/home/eernst/lang/dart/scratch/202107/n048.dart|4|3|4|Can't have a late final field in a class with a const constructor.

However, the program is accepted by the CFE (and dart runs the program, with no dynamic errors).

The error arises because Base contains a late final instance variable and a constant constructor.

The motivation for this error would be that a constant expression cannot yield an object with mutable state, and a late final instance variable in general allows for mutation after creation. However, the error is not necessary when the constant constructor is a factory constructor, as in the example above.

It might seem unnecessary to make a late final instance variable with an initializer an error when there is a generative constant constructor, because this kind of variable will only ever have one single value (and there is no setter). However, that value should be obtained when the variable is first evaluated; that evaluation could run arbitrary code, and hence it's incompatible with constant expression evaluation.

So I'd recommend that we change the spec as it is done in dart-lang/language#1764, thus relaxing this error a bit and allowing the factory constructor.

@eernstg eernstg added the type-question A question about expected behavior or functionality label Jul 29, 2021
@eernstg
Copy link
Member Author

eernstg commented Jul 29, 2021

@johnniwinther, @mkustermann, @sigmundch, I would expect this change to be small effort, or even a no-op, for the CFE, the vm, dart2js, and DDC: The above program is already accepted and executed without issues with each of these tools.

@scheglov, the analyzer currently reports the error as specified for the example program. How much work would you expect that it takes to eliminate that error? The core of the change would be to stop emitting the error when the constant constructor is a factory, but keep doing it when it is generative.

If we decide to do this then there will be a separate implementation tracking issue, with label 'implementation', and some tests.

@a-siva a-siva added the area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...). label Jul 29, 2021
@scheglov
Copy link
Contributor

scheglov commented Aug 5, 2021

Should be very easy to fix in the analyzer.
https://dart-review.googlesource.com/c/sdk/+/209222

@mkustermann
Copy link
Member

@eernstg No objects from VM team for this change. (As long as we don't allow late final for classes with generative const constructor)

@eernstg
Copy link
Member Author

eernstg commented Aug 6, 2021

Thanks, @scheglov and @mkustermann! And late final variables in a class with a generative constant constructor is definitely not going to happen!

@eernstg
Copy link
Member Author

eernstg commented Aug 6, 2021

@sigmundch, does this change (that is, dart-lang/language#1764) look benign to you as well?

@sigmundch
Copy link
Member

likewise, no concerns on our end for the web backends. /cc @fishythefish @nshahan, thanks for checking!

@fishythefish
Copy link
Member

I don't foresee any issues for dart2js.

@eernstg
Copy link
Member Author

eernstg commented Aug 24, 2021

We decided to do this, and the implementation issue is here: #46984.

@eernstg eernstg closed this as completed Aug 24, 2021
dart-bot pushed a commit that referenced this issue Aug 25, 2021
Does not pass https://dart-review.googlesource.com/c/sdk/+/211140
yet, because requires one more fix, not reporting
CONST_CONSTRUCTOR_WITH_NON_FINAL_FIELD for const redirect constructors.

Bug: #46985
Bug: #46758
Change-Id: If6289b595545216d2d4e247214c5176a6a9d4a31
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/209222
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...). type-question A question about expected behavior or functionality
Projects
None yet
Development

No branches or pull requests

6 participants