add unused_top_members_in_executable_libraries #3513

a14n · 2022-07-12T14:16:29Z

Description

Report unused top-level members in executable libraries.

coveralls · 2022-07-12T14:19:08Z

Coverage increased (+0.03%) to 95.506% when pulling 7bbd899 on a14n:unused_top_members_in_executable_libraries into f7d2da0 on dart-lang:main.

bwilkerson

It isn't clear to me that this is a rule we want to have.

As noted below, it will have false positives unless a convention is followed, and there's no support for enforcing that convention. If we think that this is a convention that we should support, then we should probably have a lint that ensures that it isn't violate. It seems to me that without that enforcement we run a risk of having a high rate of false positives from this rule.

bwilkerson · 2022-07-12T14:38:36Z

lib/src/rules/unused_top_members_in_executable_libraries.dart

+const _details = r'''
+
+Top-level members in an executable library should be used directly inside this
+library. Executable library are usually never used as dependency and it's better


nit: "library" --> "libraries"

More importantly, though, this needs to be reworded to make it clear that this lint assumes a convention in which libraries containing main are not imported by other libraries, and that there is no support for ensuring adherence to such a convention.

I added a comment regarding the assumption.

lib/src/rules/unused_top_members_in_executable_libraries.dart

bwilkerson · 2022-07-12T14:56:05Z

lib/src/rules/unused_top_members_in_executable_libraries.dart

+    return false;
+  }
+
+  bool _isInsideExecutableLibrary(AstNode node) {


The node is always a CompilationUnit, so I don't think you need the first two lines (and the parameter type should be narrowed).

Actually I changed the test to check if there is a declaration conforming the _isEntryPoint function (inlined).

bwilkerson · 2022-07-12T14:56:49Z

lib/src/rules/unused_top_members_in_executable_libraries.dart

+    if (root is! CompilationUnit) return false;
+    var library = root.declaredElement?.library;
+    return library != null &&
+        library.exportNamespace.definedNames.containsKey('main');


I think this can be replaced by library?.entryPoint != null. If so, consider inlining this method.

a14n

I ran this lint on flutter (see flutter/flutter#107464) and I didn't face false positives.

a14n · 2022-07-12T15:45:59Z

lib/src/rules/unused_top_members_in_executable_libraries.dart

+const _details = r'''
+
+Top-level members in an executable library should be used directly inside this
+library. Executable library are usually never used as dependency and it's better


I added a comment regarding the assumption.

lib/src/rules/unused_top_members_in_executable_libraries.dart

a14n · 2022-07-12T15:48:46Z

lib/src/rules/unused_top_members_in_executable_libraries.dart

+    return false;
+  }
+
+  bool _isInsideExecutableLibrary(AstNode node) {


Actually I changed the test to check if there is a declaration conforming the _isEntryPoint function (inlined).

a14n · 2022-07-22T13:58:26Z

PTAL

pq · 2022-07-27T16:16:17Z

@srawlins, since you provided motivation in #2395, could you take a look?

lib/src/rules/unused_top_members_in_executable_libraries.dart

test_data/rules/unused_top_members_in_executable_libraries.dart

srawlins · 2022-07-27T18:23:08Z

test_data/rules/unused_top_members_in_executable_libraries.dart

+// BSD-style license that can be found in the LICENSE file.
+
+// test w/ `dart test -N unused_top_members_in_executable_libraries`
+


How do you want to treat doc comments? I think they are probably not use:

class C {} /// I love [C]. void main() {}

However we decide this, there should be a test asserting the expected behavior.

I would prefer to not count it as usage, but I don't have a strong opinion.

test_data/rules/unused_top_members_in_executable_libraries.dart

srawlins · 2022-07-27T18:29:54Z

@pq is there a way to benchmark this before it is landed? I worry about performance on this one because of traverseNodesInDFS.

pq · 2022-07-27T22:53:47Z

is there a way to benchmark this before it is landed?

Great question! The good news is each PR triggers the benchmark bot and that gives us a feel. If you expand the checks, you'll see it:

Click on details and you can look at the output.

In this case, you're right that as implemented, this is relatively costly:

bwilkerson · 2022-08-02T15:37:29Z

lib/src/rules/unreachable_from_main.dart

+          element.isPublic &&
+          !element.hasVisibleForTesting;
+    });
+    unusedMembers.forEach(rule.reportLint);


Given that these are declarations, it would be better if we could report the lint on the name of the declaration, or the extension keyword in the case of an unnamed extension.

For dart-lang/linter#3513 Change-Id: Ia522fc8958c2c7c48e3366a874ceae28cb9ed6bf Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/253302 Reviewed-by: Brian Wilkerson <brianwilkerson@google.com> Commit-Queue: Samuel Rawlins <srawlins@google.com>

lib/src/rules/unreachable_from_main.dart

test_data/rules/unreachable_from_main.dart

lib/src/rules/unreachable_from_main.dart

test_data/rules/unused_top_members_in_executable_libraries.dart

lib/src/rules/unreachable_from_main.dart

a14n · 2022-08-03T08:18:51Z

PTAL

a14n · 2022-08-08T19:09:07Z

I perhaps missed some comments. Is there something blocking on this PR?

srawlins · 2022-08-03T15:04:43Z

lib/src/rules/unreachable_from_main.dart

+Top-level members in an executable library should be used directly inside this
+library.  An executable library is a library that contains a `main` top-level
+function or that contains a top-level function annotated with
+`@pragma('vm:entry-point')`).  Executable libraries are usually never imported


nit: "usually never imported" is kind of an odd construct here. Maybe something like "typically not imported" (or "not typically imported"? IANATW*), or "not usually imported". I usually lean on @bwilkerson for word-smithing.

* I am not a technical writer

srawlins · 2022-08-03T17:21:12Z

lib/src/rules/unused_top_members_in_executable_libraries.dart

+  bool _isPragmaVmEntry(Annotation annotation) {
+    var elementAnnotation = annotation.elementAnnotation;
+    if (elementAnnotation != null) {
+      var value = elementAnnotation.computeConstantValue();


That is a good test case, but we can still do a pragma element check before we evaluate the constant. Something like:

var element = annotation.elementAnnotation?.element; DartType elementType; if (element is ConstructorElement) { elementType = element.returnType; } else if (element is PropertyAccessorElement && element.isGetter) { elementType = element.returnType; } else { // Dunno what this is. return false; } if (elementType.element.name != 'pragma' || !elementType.element.library.isDartCore) { return false; }

This is similar to what analyzer does for DartType.isDartCoreBool etc in lib/src/dart/element/type.dart.

This works for the existing entryPoint annotation you have, as well as:

const entryPoint2 = entryPoint; @entryPoint2 void f77() {} // OK

It looks like pragma cannot be extended because the only public constructor is a factory constructor. It can still be implemented, but I see no code (like test cases) with extends pragma so I don't think it's important. Since pragma cannot be extended I don't think we have to worry about other constructors and return types not being exactly the pragma element.

srawlins · 2022-08-04T15:42:26Z

test_data/rules/unused_top_members_in_executable_libraries.dart

+// BSD-style license that can be found in the LICENSE file.
+
+// test w/ `dart test -N unused_top_members_in_executable_libraries`
+


However we decide this, there should be a test asserting the expected behavior.

I would prefer to not count it as usage, but I don't have a strong opinion.

srawlins · 2022-08-09T07:05:11Z

I forgot to click Send on my last 3 comments; sorry about that! I think my open comments amount to:

How do we treat references in comments?
Can we avoid evaluating the @pragma constant until we know its a @pragma annotation?

a14n · 2022-08-09T09:08:41Z

How do we treat references in comments?

There was already a test to trigger a diagnostic on elements only ref in comment (see Comment in test file)

Can we avoid evaluating the @pragma constant until we know its a @pragma annotation?

Done. Thanks for the code snippet.

a14n · 2022-08-09T09:58:59Z

After rebasing on main (and upgrading the analyzer) references in comments are now visited. I updated the test to not mark Comment as unreachable.

PTAL

srawlins

Looks great! Thanks! 🎉

srawlins · 2022-08-10T04:53:40Z

lib/src/rules/unreachable_from_main.dart

+Top-level members in an executable library should be used directly inside this
+library.  An executable library is a library that contains a `main` top-level
+function or that contains a top-level function annotated with
+`@pragma('vm:entry-point')`).  Executable libraries are not usually imported


I think there is a stray ) here, at the end of the sentence.

dart-lang/linter#3513

* add unused_top_members_in_executable_libraries * address review comments * address review comments * rename to unreachable_from_main * report on name * address review comments * address review comments * fix post-rebase

bwilkerson reviewed Jul 12, 2022

View reviewed changes

a14n commented Jul 12, 2022

View reviewed changes

pq requested a review from srawlins July 27, 2022 16:15

srawlins requested changes Jul 27, 2022

View reviewed changes

a14n force-pushed the unused_top_members_in_executable_libraries branch from aa0aa82 to 9911da4 Compare July 29, 2022 09:09

bwilkerson reviewed Aug 2, 2022

View reviewed changes

srawlins requested changes Aug 3, 2022

View reviewed changes

srawlins mentioned this pull request Aug 4, 2022

proposal: Check static members and enum members in unreachable_from_main #3581

Closed

5 tasks

srawlins reviewed Aug 9, 2022

View reviewed changes

a14n added 8 commits August 9, 2022 11:12

add unused_top_members_in_executable_libraries

7fb8f15

address review comments

4b08f5f

address review comments

da344fe

rename to unreachable_from_main

e666287

report on name

3a8e7e8

address review comments

a1d84ae

address review comments

80865bb

fix post-rebase

7bbd899

a14n force-pushed the unused_top_members_in_executable_libraries branch from 8d5f93f to 7bbd899 Compare August 9, 2022 09:54

srawlins approved these changes Aug 10, 2022

View reviewed changes

srawlins merged commit 9b1f555 into dart-lang:main Aug 10, 2022

mockturtl added a commit to mockturtl/tidy that referenced this pull request Aug 10, 2022

feat: unreachable_from_main

6e3d85b

dart-lang/linter#3513

a14n deleted the unused_top_members_in_executable_libraries branch August 28, 2022 13:51

This was referenced Sep 26, 2022

Lint top level members in tests libraries if they are not used in the library itself #685

Closed

Ban public top-level members in a test #1258

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add unused_top_members_in_executable_libraries #3513

add unused_top_members_in_executable_libraries #3513

a14n commented Jul 12, 2022

coveralls commented Jul 12, 2022 •

edited

Loading

bwilkerson left a comment

bwilkerson Jul 12, 2022

a14n Jul 12, 2022

bwilkerson Jul 12, 2022

a14n Jul 12, 2022

bwilkerson Jul 12, 2022

a14n left a comment

a14n Jul 12, 2022

a14n Jul 12, 2022

a14n commented Jul 22, 2022

pq commented Jul 27, 2022

srawlins Jul 27, 2022

srawlins Aug 4, 2022

srawlins commented Jul 27, 2022

pq commented Jul 27, 2022

bwilkerson Aug 2, 2022

a14n Aug 2, 2022

a14n commented Aug 3, 2022

a14n commented Aug 8, 2022

srawlins Aug 3, 2022

srawlins Aug 3, 2022

srawlins Aug 4, 2022

srawlins commented Aug 9, 2022

a14n commented Aug 9, 2022

a14n commented Aug 9, 2022

srawlins left a comment

srawlins Aug 10, 2022

		// BSD-style license that can be found in the LICENSE file.

		// test w/ `dart test -N unused_top_members_in_executable_libraries`

add unused_top_members_in_executable_libraries #3513

add unused_top_members_in_executable_libraries #3513

Conversation

a14n commented Jul 12, 2022

Description

coveralls commented Jul 12, 2022 • edited Loading

bwilkerson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a14n left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a14n commented Jul 22, 2022

pq commented Jul 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srawlins commented Jul 27, 2022

pq commented Jul 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a14n commented Aug 3, 2022

a14n commented Aug 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srawlins commented Aug 9, 2022

a14n commented Aug 9, 2022

a14n commented Aug 9, 2022

srawlins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 12, 2022 •

edited

Loading