Closed
Description
When the VM detects an LateInitializationError
with null safety, it throws a generic error like:
00:07 +5 -2: mergeWith Test [E]
LateInitializationError: internalHandler
package:functional_listener/src/functional_value_notifiers.dart FunctionalValueNotifier.internalHandler
package:functional_listener/src/functional_value_notifiers.dart 20:38 FunctionalValueNotifier.removeListener
package:functional_listener/src/functional_value_notifiers.dart 140:11 MergingValueNotifiers.removeListener
package:functional_listener/functional_listener.dart 223:17 ListenableSubscription.cancel
listenable_pipe_test.dart 160:18
This error does not detail which of the two cases of LateInitializationError happened: did we read something that wasn't initialized, or did we write to an already initialized final late variable.
It would be nice to either add this detail in the error output, or potentially even split LateInitializationError
into two errors (like LateUninitializedError
and LateFinalReInitializationError
).
Further it could be made a bit clearer which variable triggered the exception.
Metadata
Metadata
Assignees
Labels
Type
Projects
Relationships
Development
No branches or pull requests
Activity
mit-mit commentedon Dec 1, 2020
cc @johnniwinther
leafpetersen commentedon Dec 1, 2020
Looks like the web compilers associated a different error message with the different error cases. @alexmarkov does the VM implement this itself, or does it just pick up the front end lowering of late? Is there anything stopping the VM from calling a different constructor in the two different cases?
alexmarkov commentedon Dec 1, 2020
This is implemented in the VM (CFE lowering is not used for late fields/variables in the VM). We can throw different exception types or we can adjust the exception messages - please suggest what exactly we should do. Also, it would be nice to unify that across all Dart implementations (VM, dart2js, DDC).
leafpetersen commentedon Dec 1, 2020
Yes, I think consistency would be good. I believe the web compilers both use the implementation in lib/internal/errors.dart (is that right @nshahan @sigmundch ). So if we're happy with just making the error message consistent, then we maybe we could either consolidate those implementations or make them consistent?
cc @mit-mit @lrhn @munificent @natebosch @jakemac53 for thoughts on whether custom error messages are good enough or whether there's a justification for doing the additional work to specify and implement separate error classes.
nshahan commentedon Dec 1, 2020
FWIW @rakudrama and I have discussed opting out of the late lowering in some cases where we could do something more efficient in JavaScript.
This is true for DDC.
rakudrama commentedon Dec 2, 2020
For dart2js we will move away from using the default code patterns and Error classes.
This will be done on a case-by-case basis (e.g. treating late, late final, initialized, uninitialized, local, field, static independently).
The code size tax of the current implementation is too high (see #43361), so we want to be able to generate JavaScript where it is not possible to tell what kind of LateError is generated, or what the late variable was called, or even if the problem is a LateError or some other kind of Error. For production code we don't want to leak the names, and forcing a null dereference (NoSuchMethodError) instead of a LateError is an order of magnitude more compact.
We will try to generate accurate, actionable messages where possible, but need the freedom to do something else to make the
late
feature viable when widely used in production.leafpetersen commentedon Dec 2, 2020
Ok, so it sounds like there are no issues with making DDC and the VM consistent, and dart2js in production at least will diverge significantly no matter what. Does that seem reasonable to all? Are we happy with just making sure that there are good error messages associated with the exception (as currently implemented by DDC)?
lrhn commentedon Dec 2, 2020
Do we want to underspecify the error?
We currently specify that a later initialization error must throw an instance of
LateInitializationError
. As I read it, dart2js wants to diverge from that specification, which suggests that maybe we shouldn't actually specify which error is throw, and should instead just say that "a run-time error occurs" and leave it up to the implementations which instance of a subclass ofError
they want to actually throw. (If we do so, we should removeLateInitializationError
, it's confusing if it exists, but isn't used by dart2js).(The
LateInitializationError
is not a "default error", it's a spec mandated error, and throwing something not implementing that interface is a spec violation.)leafpetersen commentedon Dec 3, 2020
Per discussion in the language meeting today, we would like to remove LateInitializationError from the public API, and loosen the specification to simply specify that some error is thrown. The VM and DDC may choose (hopefully will choose) to implement this consistently, e.g by using the current mechanism.
mit-mit commentedon Dec 3, 2020
As of the fix above, the new output is: