Previously, if the preview server couldn't be reached because it had
exited, we would generate an error message with an obscure looking
stacktrace. I believe this was prompting users to file bug reports
against the migration tool, when in fact all they needed to do was
restart the migration process. Hopefully this better error message
should help avoid confusion.
I also took the opportunity to improve the error message that is shown
if the user does restart the migration tool but tries to keep
interacting with it using an old authToken.
Fixes#45202.
Bug: https://github.com/dart-lang/sdk/issues/45202
Change-Id: I83e0ca99d789739e614bfff2d535d427a2fdfbb2
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/192732
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Flow analysis stopped using this argument (the AST node for the
finally block) some time ago, but I kept it around so I could assert
that clients didn't unnecessarily store assigned variables info for
it. It's been long enough now that we can eliminate this code
entirely.
Change-Id: I8c64e2b4fc5b154f441ec2d057637f3dc9ced277
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/190060
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
This CL plumbs the types of `this` and property get expression from
the CFE and analyzer into flow analysis, so that flow analysis will be
able to create more accurate "why not promoted" information for those
expression types. This made it possible to eliminate a clumsy aspect
of the previous implementation, namely that we would consider a
promotion attempt like `if (x.y == null) return;` as an attempt to
promote the type of `x.y` to `Object`; now we compute the type the
user is actually trying to promote to, so we will be able to generate
more accurate "why not promoted" messages.
Bug: https://github.com/dart-lang/sdk/issues/44898
Change-Id: I67f9fc59e72103194a1ea6b1c4dfeae8aeb194a2
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/187064
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
This CL implements the core flow analysis infrastructure for tracking
reasons why an expression was not promoted. It supports the following
reasons:
- Expression was a property access
- Expression has been written to since it was promoted
I expect to add support for other non-promotion reasons in the future,
for example:
- `this` cannot be promoted
- Expression has been write captured
- Expression was a reference to a static field or top level variable
These non-promotion reasons are plumbed through to the CFE and
analyzer for the purpose of making errors easier for the user to
understand. For example, given the following code:
class C {
int? i;
f() {
if (i == null) return;
print(i.isEven);
}
}
The front end now prints:
../../tmp/test.dart:5:13: Error: Property 'isEven' cannot be accessed on 'int?' because it is potentially null.
Try accessing using ?. instead.
print(i.isEven);
^^^^^^
Context: 'i' refers to a property so it could not be promoted.
Much work still needs to be done to round out this feature, for example:
- Currently the analyzer only shows the new "why not promoted"
messages when the "--verbose" flag is specified; this means the
feature is unlikely to be noticed by users.
- Currently the analyzer doesn't show a "why not promoted" message
when the non-promotion reason is that the expression is a property
access.
- We need one or more web pages explaining non-promotion reasons in
more detail so that the error messages can contain pointers to them.
- The analyzer and front end currently only show non-promotion reasons
for expressions of the form `x.y` where `x` fails to be promoted to
non-nullable. There are many other scenarios that should be
handled.
Change-Id: I0a12df74d0fc6274dfb3cb555abea81a75884231
Bug: https://github.com/dart-lang/sdk/issues/38773
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/181741
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Previously, we did not null check the listener, which meant that if an
exception occurred in a unit test, there would be a cascading
exception, so we wouldn't get a useful stack trace.
Change-Id: I612ca9e7129441e319d92953c481fd1647eb46ef
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/180383
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Since `this` is essentially a variable in extensions, it should be
treated similarly to variables in terms of inference of non-null
intent; for example, if an extension declaration contains a a method
that unconditionall dereferences `this` (either implicitly or
explicitly), that's a good indication that the user probably doesn't
intend for the extension to be applied to nullable types.
Fixes#44675.
Bug: https://github.com/dart-lang/sdk/issues/44675
Change-Id: I004328f5b1fd6710954363c07f4e9db6bc6ac2cd
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/180268
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
This partially addresses #44675 by ensuring that if an extension's
extended type is nullable, we won't try to insert an unnecessary null
check at the call site.
It's not a complete fix, though, because we still don't account for
obvious indications of non-null intent when analyzing the extension
definition itself. I plan to address that in future CLs.
Bug: https://github.com/dart-lang/sdk/issues/44675
Change-Id: Ia0ca37b89470a2d4882ae32cd842552ffd34930e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/180263
Reviewed-by: Samuel Rawlins <srawlins@google.com>
After a variable is write captured, references to it should no longer
be considered to express non-null intent, because we can no longer
guarantee that an initially null value of the variable would
definitely lead to an exception.
Change-Id: I44c65350291a4c44cdb4b19529b13149d8170e34
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/180100
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Previously, code inside of local functions was treated as though it
appeared inline for the purpose of determining non-null intent. As a
result, a return statement in such a local function would cause any
code references that followed to be incorrectly treated as *not*
expression non-null intent.
Fixes#44703.
Bug: https://github.com/dart-lang/sdk/issues/44703
Change-Id: Ia74da007eb5424c784d41afeb0e2da4771578553
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/180081
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Previously, the nullability graph propagation algorithm identified
non-null intent prior to inference of `late`. As a result, late
inference would be skipped for any variable with clear non-null
intent, so late inference was a lot less effective than it should be.
This requires updating several test cases where `late` should have
been inferred.
Change-Id: I0412ee7045e654185af08cc12b6ca1e6866112af
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/180080
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Now that `MigrationCliRunner.shouldBeMigrated` has the correct
signature and internal clients no longer use
`MigrationCliRunner.shouldBeMigrated2`, we can remove
`MigrationCliRunner.shouldBeMigrated2`.
Change-Id: I33bdc78a603c859dd8bf1928f2dbc23233ae5e7c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/178763
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
The new implementation uses the same API as flow analysis. This
should allow us to significantly simplify the CFE and analyzer, by
dropping their implementations of legacy type promotion in favor of
the shared implementation.
This CL just introduces the new implementation and unit tests for it;
it does not integrate it with the analyzer or CFE. I will follow up
with a CL that does the integration.
Change-Id: Ie07b3b39604d6a022ad42f3ae6b648a317c8af28
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/179560
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
This is non-breaking since `shouldBeMigrated` is unreferenced outside
the nnbd_migration package). Once this lands, we can switch clients
from overriding/calling `shouldBeMigrated2` back to `shouldBeMigrated`
and then eliminate `shouldBeMigrated2`.
Change-Id: I1235113bda6f7609de17ec05e7120b312f201536
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/178762
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Paul Berry