Skip to content
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

[lint] [omit_obvious_local_variable_types] false positive #59957

Open
stephane-archer opened this issue Jan 22, 2025 · 3 comments
Open

[lint] [omit_obvious_local_variable_types] false positive #59957

stephane-archer opened this issue Jan 22, 2025 · 3 comments
Labels
analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. linter-false-positive

Comments

@stephane-archer
Copy link

(DropDoneDetails dropDoneDetails) async {
          LutFoldersNotifier lutFoldersNotifier =
              await ref.read(lutFoldersProvider.notifier);
          var paths = dropDoneDetails.files.map((e) {
            return e.path;
          });
          for (String path in paths) {
            if (FileSystemEntity.isDirectorySync(path)) {
              await lutFoldersNotifier.addLutDir(path);
            }
          }
        }

Omit the type annotation on a local variable when the type is obvious. Try removing the type annotation.

for me, there is nothing obvious here that makes path a String except the name.
So I think this is a false positive for omit_obvious_local_variable_types

@stephane-archer stephane-archer added the area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. label Jan 22, 2025
@bwilkerson
Copy link
Member

@eernstg

@bwilkerson bwilkerson added analyzer-linter Issues with the analyzer's support for the linter package linter-false-positive labels Jan 22, 2025
@eernstg
Copy link
Member

eernstg commented Jan 23, 2025

The reason why the type of path is classified as obvious by the lint is that it is an iteration variable in a for-each statement whose iterable is also classified as having an obvious type.

The iterable is a local variable, paths, which has not been promoted. A non-promoted local variable is classified as having an obvious type. This happens just because it's a local variable which isn't promoted, and there's no attempt to ensure that said local variable has itself been declared in a way that actually makes its type obvious.

(If you want that then specify_nonobvious_local_variable_types will do the job.)

The underlying rationale is that the developer who wrote that local variable declaration (of paths, in this case) would have made sure that the type of the local variable is as evident as it should be, and the variable itself is henceforth considered to be obviously typed (unless it is promoted, which is always considered to be non-obvious).

In short, when it comes to local variables that depend on each other, the rule of thumb is as follows: "If some of these variables need a type annotation then go to the root cause rather than fixing it downstream". This would make the code more evidently typed overall with a small number of type annotations (most dependency trees have more nodes near the leaves than near the root).

We could adopt other approaches. E.g., the type of a local variable could be considered to be non-obvious if its declaration has an initializing expression whose type isn't obvious (which could then rely on recursive queries in cases like var v4 = [v1, v2, v3];). However, I do think it's a better general strategy to maintain a style where no local variable has an obscure type (which is a matter of human judgment), and every local variable that hasn't been promoted is considered to have an obvious type.

@stephane-archer
Copy link
Author

stephane-archer commented Jan 23, 2025

every local variable that hasn't been promoted is considered to have an obvious type.

We can see with paths that it is not always the case.

If paths type was obvious I would agree with your lint but because here var is used it is not obvious. Your assumption about local variable obviousness is not always right.

I think you should revisit this assumption and see if var is used on local variables before deciding it's obvious

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. linter-false-positive
Projects
None yet
Development

No branches or pull requests

3 participants