add use_late lint #2189

a14n · 2020-07-16T21:16:52Z

Description

Private members with nullable type can be replaced by non-nullable type and late if all the access are nullsafe.

BAD:

int? _i;
m() {
  _i!.abs();
}

GOOD:

late int _i;
m() {
  _i.abs();
}

bwilkerson

@leafpetersen Is this something that some users are likely to want to enforce? Not having had an opportunity to actually use null safety features yet, it isn't immediately obvious to me.

bwilkerson · 2020-07-16T21:43:29Z

lib/src/rules/use_late.dart

+
+const _details = r'''
+
+Use late for private members with non-nullable type to avoid null checks.


Are we attempting to avoid null checks in order to improve performance or for readability?

Mainly readability because I don't have number about perf (as there are less null checks I guess it will be better).

Based on Erik's comment, it sounds like code should have about the same performance either way. It would be good to clarify in the details that the purpose is to make it clear in the code that a field / variable is not expected to be null. Users who are new to null safety might not yet understand the value of late.

Comment updated

Definitely readability.

It makes some checks implicit that you would otherwise have to write boiler-plate code for. That should make the actually important parts of the code stand out more.

It's unlikely to be measurably more performant in general.
In some cases we might be able to eliminate the initialization checks of a late variable (say when it's initialized in the body of the only generative constructor), but then, we could probably eliminate null checks in the same cases if we wanted to.

bwilkerson · 2020-07-16T21:44:03Z

lib/src/rules/use_late.dart

+  void visitFieldDeclaration(FieldDeclaration node) {
+    for (var variable in node.fields.variables) {
+      if (Identifier.isPrivateName(
+              (node.parent as ClassOrMixinDeclaration).name.name) ||


I believe that it's possible to define a static field in an extension, in which case the node's parent will be an ExtensionDeclaration and this cast will fail.

Updated to handle extensions

bwilkerson · 2020-07-16T21:44:35Z

lib/src/rules/use_late.dart

+      return;
+    }
+    final myVisitor = _MyVisitor(context, variable);
+    myVisitor.visitCompilationUnit(_compilationUnit);


This seems very inefficient. Consider writing a single visitor that will collect all of the nodes representing the declaration of a private top-level variable or private field and all of the nodes representing references to private variables or fields, and then compare the two lists after the visitor has returned.

bwilkerson · 2020-07-16T21:44:46Z

lib/src/rules/use_late.dart

+        DartTypeUtilities.getCanonicalElementFromIdentifier(node) ==
+            variable.declaredElement) {
+      var parent = node.parent;
+      while (parent is ParenthesizedExpression) {


Consider using Expression.unParenthesized.

Done but the code isn't really clearer (I don't know why the inference failed to pormote to Expression)

bwilkerson · 2020-07-16T21:44:52Z

test/rules/experiments/nnbd/rules/use_late.dart

@@ -0,0 +1,49 @@
+// Copyright (c) 2016, the Dart project authors. Please see the AUTHORS file


'2016' --> '2020'

leafpetersen · 2020-07-17T01:32:17Z

This overall looks like a reasonable lint to me. I didn't look at the implementation - will if fire on the example below (and if so, is that considered an ok "false positive")?

class A {
  int? x;
  int? compute(bool returnNull) {if (returnNull) return null; return 0; }
  void test() {
     x = compute(false);  // if x is made late int x, this will require a ! to be inserted
     print(x!.isEven);
  }

If you're not ok with the lint firing on the above, then you could also check that there are no nullable assignments to x.

cc @natebosch @lrhn @munificent for their thoughts

a14n · 2020-07-17T07:14:38Z

@leafpetersen The lint will only apply on private members (or public members inside private class). In your example with _x instead of x the lint will not appear because _x is assigned with a nullable type (_x = compute(false)).

eernstg · 2020-07-17T13:45:20Z

Interesting! It would work for any private non-local variable. However, I think a couple of things would have to be a bit more strict:

The privacy constraint would be needed in order to ensure that a scan of a known amount of source code is guaranteed to come up with all accesses. But then it isn't safe to include public instance members inside a private class, because they could be accessed dynamically, and if that class has a public subclass then they can even be accessed statically.

But if every write to the variable has a non-nullable type, and every read of the variable is guarded by a throwing null check (x! will do, and so will x as int, but if (x != null) ... will not) then we're guaranteed that every case where the transformed code will throw because it is reading a late variable that has not been initialized would also have thrown in the original code. (The transformation would be manual because this is a lint, and it may or may not be tractable to transform the code automatically.)

So this should be sound, and it's a really nice property that the transformation reveals a stronger discipline in the use of that variable: Before the transformation it could be null at any time, as far as a reader of the code can see, but after the transformation it is known that the variable will start off as uninitialized, and when it has been initialized it will never go back. We don't get any extra static guarantees about that initialization, but that's the same, not worse.

The performance should be the same (being uninitialized can be represented by having the value null, for a late variable with a non-nullable type).

So it looks like it would be an improvement whenever it's applicable.

bwilkerson · 2020-07-17T14:15:12Z

lib/src/rules/use_late.dart

+
+const _details = r'''
+
+Use late for private members with non-nullable type to avoid null checks.


Based on Erik's comment, it sounds like code should have about the same performance either way. It would be good to clarify in the details that the purpose is to make it clear in the code that a field / variable is not expected to be null. Users who are new to null safety might not yet understand the value of late.

bwilkerson · 2020-07-17T14:17:37Z

lib/src/rules/use_late.dart

+class UseLate extends LintRule implements NodeLintRule {
+  UseLate()
+      : super(
+          name: 'use_late',


Given that this only applies to private fields or top-level variables, we should make that explicit in the name so that when/if we decide to add a similar lint for local variables they are clearly differentiated (use_late_for_private_fields_and_variables? Hopefully you can come up with something better).

bwilkerson · 2020-07-17T14:30:41Z

lib/src/rules/use_late.dart

+  }
+}
+
+bool _isPrivateNamedCompilationUnitMember(AstNode parent) =>


Per Erik's comments, if we want to support public fields in private classes, we need to also ensure that there are no instances of either the enclosing class or any subclass of the enclosing class that are ever accessible outside this library.

I suspect that it isn't worth the effort to include these cases, at least at this point, but either way it would be worth capturing this idea and the requirements for doing so in a comment somewhere in this file.

Comment added

bwilkerson · 2020-07-17T14:39:55Z

lib/src/rules/use_late.dart

+  @override
+  void visitCompilationUnit(CompilationUnit node) {
+    if (node.featureSet.isEnabled(Feature.non_nullable)) {
+      super.visitCompilationUnit(node);


This only works for libraries without parts. To fix this, it needs to visit all of the compilation units in the library, but it needs to gather "lateable" variables only while visiting the current compilation unit but gather "nullableAccess"es in all of the units.

bwilkerson · 2020-07-17T15:34:18Z

lib/src/rules/use_late.dart

+  final LinterContext context;
+
+  final lateables = <VariableDeclaration>[];
+  final nullableAccess = <AstNode, Element>{};


I saw this earlier, but forgot to comment on it.

We don't really need to keep track of the element accessed at a given node, just just need to know, for each element, whether we have found a nullable access. Consider using the following instead:

final nullableAccess = <Element>{};

and just adding elements for which a nullable access was found to the set. Checking whether an element is "lateable" is then just a question of checking that it isn't in the set. That should keep this collection much smaller which will also improve performance.

I keep the node to check the offset and evict every node at the same offset as the variable.

The other (and in my opinion better) option would be to not add the elements associated with simple identifiers that are inDeclarationContext().

natebosch · 2020-07-17T18:58:19Z

This makes intuitive sense to me. It would be nice to run this across already migrated packages and see if it catches anything. I don't have a sense for whether this is something that folks are likely to need help with.

@a14n - do you have real world motivating examples for this from the flutter use case? Something where you would have done it a different way and it was found in code review that late is nicer?

a14n · 2020-07-17T20:48:16Z

I'd like to run this lint on the flutter parts I already migrated to see where this lint triggers but it looks like there's a problem somewhere and the following command doesn't work correctly:

# with the branch of this PR
~/flutter$ bin/cache/dart-sdk/bin/dart $LINTER_HOME/bin/linter.dart --packages "packages/flutter/.dart_tool/package_config.json" --rules use_late packages/flutter

flutter/packages/flutter/test/cupertino/activity_indicator_test.dart 36:58 [static warning] Undefined name 'Brightness'.
          data: const MediaQueryData(platformBrightness: Brightness.light),
                                                         ^^^^^^^^^^

It looks like the issue is caused by dart:ui not being resolved. Someone has an idea?

do you have real world motivating examples for this from the flutter use case?

When I manually migrated some files I almost always used a nullable type on every field because it's hard to audit every usage for each field (but now you need to put ! everywhere). This lint would help to reduce all those ! and make the code better/nicer.

leafpetersen · 2020-07-17T20:55:53Z

When I manually migrated some files I almost always used a nullable type on every field because it's hard to audit every usage for each field (but now you need to put ! everywhere). This lint would help to reduce all those ! and make the code better/nicer.

I would +1 this. It takes a very careful reading to notice that all uses are explicitly null checked. During the migration, I was being very careful to try to do this manual audit, and did find some cases (and I think found some in code reviews as well - I think maybe in the dart:ui migration?), but in general it's easy to miss.

I wonder whether it would be useful to make this apply to local variables as well?

natebosch · 2020-07-17T21:03:29Z

Should we be looking at a change in the migration tool to prefer late?

Do we expect this lint to primarily have value during the migration, or do we expect it will have value in the long term?

leafpetersen · 2020-07-17T21:10:42Z

Do we expect this lint to primarily have value during the migration, or do we expect it will have value in the long term?

It feels useful to me long term, especially if it applies to locals. If you're editing some code and eliminate the last unchecked use of a nullable variable (or last assignment of a nullable value to a nullable variable) then this seems like a good change to consider.

This also makes me wonder whether there should be a parallel lint that find unnecessary uses of late ? That is, variables which are marked late, but which could be made just regular variables using definite assignment?

bwilkerson · 2020-07-17T21:26:00Z

This also makes me wonder whether there should be a parallel lint that find unnecessary uses of late ?

That's a good question. I don't know how often users will run into that situation, but I suspect that it will be fairly difficult to detect without some level of automation, so I'd be inclined to say yes.

Is there a performance penalty related to marking a local variable as late? (Erik suggested earlier that there shouldn't be any penalty for non-nullable fields, so I'd guess that it's the same here, but I could easily be wrong about that.)

If nothing else, the code would be cleaner with extra lates.

leafpetersen · 2020-07-17T21:41:14Z

Is there a performance penalty related to marking a local variable as late?

I'm very hesitant to speculate about the performance here. Absent optimization, a non-nullable variable marked late will have a performance penalty relative to a definitely assigned non-nullable variable not marked late. Optimization may (or may not) eliminate this penalty. It may not do so now, but may later once optimizations are more mature. Conversely, in principle late + non-nullable is more easily optimizable than non-late + nullable with explicit null checks. But post optimization they may (or may not) be the same.

If nothing else, the code would be cleaner with extra lates.

Yes. I'd argue it's also more robust. With late, you won't get warned if you forget to assign on one path (that's the point of late - you get dynamic checking instead of static). Without late, you will get warned if a subsequent refactor accidentally eliminates an initialization on some path.

bwilkerson · 2020-07-20T17:18:18Z

lib/src/rules/use_late_for_private_fields_and_variables.dart

+      super.visitCompilationUnit(node);
+
+      final units = node.declaredElement.library.units;
+      if (units.every(lateables.keys.contains)) {


I'm not positive that there's an issue here, but it might be the case that generated files won't be visited, which would mean that libraries with generated parts wouldn't ever satisfy this condition. (And if it's the case today that all parts are always visited, there's no reason to think that they always will be.)

I guess what I was thinking about was more along the lines of

if (node is a library and nnbd is enabled for the library) { for (each unit in `LinterContext.allUnits`) visit the unit to gather lateables (in per unit caches) and nullableAccesses (in a single collection) for (each unit in `LinterContext.allUnits`) create lints for any lateable that isn't referenced in a nullable access

bwilkerson · 2020-07-20T17:19:04Z

lib/src/rules/use_late_for_private_fields_and_variables.dart

+    for (final unit in units) {
+      for (var variable in lateables[unit]) {
+        if (!allNullableAccess.contains(variable.declaredElement)) {
+          rule.reportLint(variable);


I believe that this will report the lint for variables declared in one file at the offset of the variable, but in the file currently being visited, which might be a different file. As far as I know, the linter doesn't have good support for reporting lints across a whole library.

What you'll probably need to do is use reporter.reportError and pass in an AnalysisError that is created here (rather than use reportLint which creates the AnalysisError for you).

We probably need some tests of libraries with parts to ensure that this is working as intended.

bwilkerson · 2020-07-20T17:27:37Z

lib/src/rules/use_late_for_private_fields_and_variables.dart

+// Ideally we need to also ensure that there are no instances of either the
+// enclosing class or any subclass of the enclosing class that are ever
+// accessible outside this library.
+bool _isPrivateNamedCompilationUnitMember(AstNode parent) =>


I think I didn't communicate clearly. Sorry about that.

What I was trying to say is that Erik pointed out a source of false positives that I suspect will result in enough false positives that we need to get rid of them. I think there are two ways to remove the false positives. One is by doing the work to prove that no instance of the class can escape the library, but that this approach is probably not worth the effort. The other is by not attempting to lint publicly named members of private classes.

The suggestion to add a comment was intended to capture the fact that if we did the extra analysis we could extend the lint to also look for publicly named members of private classes.

Of course, I might be wrong about the false positives, but I don't know whether we have enough ported code to really gather data in this case.

a14n · 2020-07-21T15:42:26Z

@pq any advice about how to run my PR on flutter and workaround the issue mentioned in an above comment ?

pq · 2020-07-22T05:22:55Z

The linter.dart binary is really not very smart and I'm not surprised that you're seeing resolution issues (dart:ui is kind if a clever special case IIRC). That said, you can probably safely ignore the unresolved type warnings and still get a sense for how flutter will analyze.

Alternatively, if you're setup to build the Dart SDK, you could update DEPS to point to this commit, enable the lint in analysis options and run the dartanalyzer on the sources.

As for a workaround, which issue specifically?

a14n · 2020-07-22T06:46:44Z

As for a workaround, which issue specifically?

Sorry to not mentioned the real issue, running linter.dart doesn't detect that Feature.non_nullable is enabled for migrated files and so no lint is displayed.

a14n · 2020-07-23T15:50:46Z

Alternatively, if you're setup to build the Dart SDK, you could update DEPS to point to this commit, enable the lint in analysis options and run the dartanalyzer on the sources.

I didn't manage to run the dartanalyzer (build with updated linter) on flutter either directly:

~/flutter/packages/flutter$ ~/dart/dart-sdk/sdk/out/ReleaseX64/dart-sdk/bin/dartanalyzer --enable-experiment=non-nullable --packages .dart_tool/package_config.json  .
Analyzing flutter...
Unhandled exception:
FormatException: The resolved 'packageUri' must be inside the rootUri, actually 'lib/' is not in './', for the package 'flutter'.
{
  "configVersion": 2,
  "packages": [
    {
      "name": "async",
      ...
#0      _PackageConfigJsonParser._parsePackage (package:analyzer/src/context/package_config_json.dart:199:9)
#1      _PackageConfigJsonParser._parsePackages (package:analyzer/src/context/package_config_json.dart:243:9)
#2      _PackageConfigJsonParser.parse (package:analyzer/src/context/package_config_json.dart:116:7)
#3      parsePackageConfigJson (package:analyzer/src/context/package_config_json.dart:13:49)
#4      parsePackageConfigJsonFile (package:analyzer/src/context/packages.dart:73:20)
#5      parsePackagesFile (package:analyzer/src/context/packages.dart:122:12)
#6      ContextBuilder.createPackageMap (package:analyzer/src/context/builder.dart:189:14)
#7      ContextBuilder.createWorkspace (package:analyzer/src/context/builder.dart:379:35)
#8      ContextBuilder.getAnalysisOptions (package:analyzer/src/context/builder.dart:272:24)
#9      ContextCacheEntry._getAnalysisOptions (package:analyzer_cli/src/context_cache.dart:105:34)
#10     ContextCacheEntry.analysisOptions (package:analyzer_cli/src/context_cache.dart:81:28)
#11     HasContextMixin.createAnalysisOptionsForCommandLineOptions (package:analyzer_cli/src/has_context_mixin.dart:27:44)
#12     Driver._createContextAndAnalyze (package:analyzer_cli/src/driver.dart:508:9)
#13     Driver._analyzeAllImpl (package:analyzer_cli/src/driver.dart:254:9)
<asynchronous suspension>
#14     Driver._analyzeAll (package:analyzer_cli/src/driver.dart:189:20)
<asynchronous suspension>
#15     Driver.start (package:analyzer_cli/src/driver.dart:147:28)
#16     main (file:///home/a14n/dart/dart-sdk/sdk/pkg/analyzer_cli/bin/analyzer.dart:18:17)
#17     _startIsolate.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:297:32)
#18     _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:168:12)

or with a custom sdk passed to flutter analyze

$ flutter analyze --dart-sdk=/home/a14n/dart/dart-sdk/sdk/out/ReleaseX64/dart-sdk
Analyzing flutter...                                                    
  error • double isn't a type • lib/src/animation/animation_controller.dart:235:5 • not_a_type
  error • This requires the 'non-nullable' language feature to be enabled • lib/src/animation/animation_controller.dart:235:11 • experiment_not_enabled
[....]

It's hard to make a lint without being able to test it on real code. :-(

a14n · 2020-07-28T20:56:12Z

/ping

pq · 2020-07-28T21:32:09Z

@scheglov: any thoughts on the _PackageConfigJsonParser error seen above?

scheglov · 2020-07-29T23:39:52Z

OK, I can reproduce it by running dartanalyzer command line that you provided.

Actually, we have a bug earlier, and we have a guard, but I guess we were shy and did not enable it always, only as an assertion. So, we surprisingly fail later. I'm going to make it a hard exception, and fix analyzer_cli in places where it fails into it.

As a workaround pass the absolute path for --packages.

a14n · 2020-07-30T06:39:59Z

@scheglov Thanks for the workaround, it works great and I can now test this lint on flutter!

a14n · 2020-07-30T13:49:47Z

After applying the lint on flutter migrated code it works quite well and has detected several places where late can be used.

bwilkerson · 2020-07-30T14:27:29Z

lib/src/rules/use_late_for_private_fields_and_variables.dart

+  }
+
+  @override
+  void visitFieldDeclaration(FieldDeclaration node) {


I believe that this needs to invoke super.visitFieldDeclaration(node) so that the initializer will be visited because late instance fields can reference other instance members.

bwilkerson · 2020-07-30T14:28:06Z

lib/src/rules/use_late_for_private_fields_and_variables.dart

+  }
+
+  @override
+  void visitTopLevelVariableDeclaration(TopLevelVariableDeclaration node) {


I believe that this needs to invoke super.visitTopLevelVariableDeclaration(node) for a similar reason.

bwilkerson · 2020-07-30T14:33:07Z

For the record, I opened #2205 to capture Leaf's lint idea.

bwilkerson · 2020-07-30T15:05:26Z

It would be good to have tests for those cases at some point, but I'm OK with having this land so that you can start using it.

See dart-archive/linter#2189 (comment) Change-Id: I6becb616dd8bcb0c9945ea5ad5ac6119bb93e330 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/156441 Reviewed-by: Brian Wilkerson <[email protected]> Commit-Queue: Konstantin Shcheglov <[email protected]>

lib/src/rules/use_late_for_private_fields_and_variables.dart

pq

Assuming Brian's comments about super calls are addressed, LGTM!

pq

🎉

Thanks everyone for all the thoughtful back and forth!

* add use_late lint * address review comments * address review comments * handle parts * rename to use_late_for_private_fields_and_variables * address review comments * address review comments * address review comments

add use_late lint

2a7e90d

googlebot added the cla: yes label Jul 16, 2020

bwilkerson reviewed Jul 16, 2020

View reviewed changes

address review comments

f4d0548

bwilkerson reviewed Jul 17, 2020

View reviewed changes

address review comments

ccab41d

handle parts

4990026

leafpetersen mentioned this pull request Jul 17, 2020

[nnbd] Late Keyword is Redundant for Local Non-nullable variables without Intializers dart-lang/language#1101

Open

rename to use_late_for_private_fields_and_variables

ed1434d

bwilkerson reviewed Jul 20, 2020

View reviewed changes

address review comments

649be97

bwilkerson reviewed Jul 30, 2020

View reviewed changes

address review comments

25d0a27

bwilkerson approved these changes Jul 30, 2020

View reviewed changes

pq reviewed Jul 30, 2020

View reviewed changes

lib/src/rules/use_late_for_private_fields_and_variables.dart Show resolved Hide resolved

pq approved these changes Jul 30, 2020

View reviewed changes

address review comments

30a1e72

pq approved these changes Jul 30, 2020

View reviewed changes

pq merged commit 4d3f8a1 into dart-archive:master Jul 30, 2020

a14n deleted the use_late branch September 12, 2020 13:29


		const _details = r'''

		Use late for private members with non-nullable type to avoid null checks.

		@@ -0,0 +1,49 @@
		// Copyright (c) 2016, the Dart project authors. Please see the AUTHORS file

add use_late lint #2189

add use_late lint #2189

Uh oh!

Conversation

a14n commented Jul 16, 2020

Description

Uh oh!

bwilkerson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lrhn Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leafpetersen commented Jul 17, 2020

Uh oh!

a14n commented Jul 17, 2020

Uh oh!

eernstg commented Jul 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natebosch commented Jul 17, 2020

Uh oh!

a14n commented Jul 17, 2020

Uh oh!

leafpetersen commented Jul 17, 2020

Uh oh!

natebosch commented Jul 17, 2020

Uh oh!

leafpetersen commented Jul 17, 2020

Uh oh!

bwilkerson commented Jul 17, 2020

Uh oh!

leafpetersen commented Jul 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lrhn Jul 21, 2020 •

edited

Loading

scheglov commented Jul 29, 2020 •

edited

Loading