File with lower_case_with_underscores identifier message is persisting.

After #5270 (I think this might be the cause), if you make a code like this (no actual file with this name):

export 'MyFile.dart';

You'll see the new message for it. But even if you replace this export completely (no second file needed) the message will persist.

The file name 'MyFile.dart' isn't a lower_case_with_underscores identifier.
Try changing the name to follow the lower_case_with_underscores style.
You may need to close the file and restart VS Code after renaming a file by only casing.

This is persisted because the message is showing on a non-existing file.

I don't seem to be able to reproduce this without MyFile.dart existing:

Are you able to reproduce this in a clean project? Did MyFile.dart previously exist? (is it possible you deleted it from disk, but VS Code still has it "open but deleted" so we have an in-memory overlay for it?)

Are you able to reproduce this in a clean project?

That was a clean project.

Did MyFile.dart previously exist?

Not to my knowledge. I could repro this with some different file names.

Are there any logs that would help with this?

Are there any logs that would help with this?

Yeah, the instrumentation log will help. But exact steps to reproduce in a new project would also help - it'll be easier to debug if I can repro it versus trying to reverse engineer the logs :-)

Forgot about posting these:

Workspace Environment

Dart Code extension: 3.101.20241118
Flutter extension: 3.101.20241031 (activated)

App: Visual Studio Code
App Host: desktop
Version: win 1.95.3

Workspace type: Dart (LSP)
Workspace name: message_error

Dart (3.7.0-edge.d75c493e27f9bf2a89043860132462be665065c2): C:\Users\felip_0vh5fa6\dart
Flutter (3.24.4): C:\Users\felip_0vh5fa6\.puro\envs\stable\flutter (No device)

Output from 'dart info'

C:\Users\felip_0vh5fa6\dart\bin\dart.exe info

If providing this information as part of reporting a bug, please review the information
below to ensure it only contains things you're comfortable posting publicly.

General info

Dart 3.7.0-edge.d75c493e27f9bf2a89043860132462be665065c2 (main) (Tue Nov 12 10:33:29 2024 +0000) on "windows_x64"
on windows / "Windows 11 Pro" 10.0 (Build 22631)
locale is pt-BR

Project info

sdk constraint: '^3.7.0-edge.d75c493e27f9bf2a89043860132462be665065c2'
dependencies:
dev_dependencies: lints, test

Process info

Memory	CPU	Elapsed time	Command line
0 MB	--		dart.exe
0 MB	--		dart.exe
0 MB	--		dart.exe
0 MB	--		dart.exe
0 MB	--		dart.exe
0 MB	--		dart.exe
0 MB	--		dart.exe
0 MB	--		dart.exe
0 MB	--		dart.exe
0 MB	--		dart.exe
0 MB	--		dart.exe

New empty dart project:

20241119_094631.mp4

Here are the full logs:

message_error.zip

Thanks! I can repro following your video.

Btw, your analysis seems to update incredibly slowly for such a small project (it takes a long time for the error about e being undefined to be updated). Is this the kind of performance you normally see? For me, it updates as fast as I type:

Recording.2024-11-26.143546.mp4

Btw, your analysis seems to update incredibly slowly for such a small project (it takes a long time for the error about e being undefined to be updated). Is this the kind of performance you normally see?

Yes, it is. I don't have all of the folders you have set to ignore for my antimalware software, maybe that is it? As you noticed, this is a small (empty) project but the time doesn't really go up on larger ones (not that I've noticed at least) so, I'm used to it at this point.

If you have any tips I'm open to them.

(The above was with the analyzer from source but I've just tested with the snapshot and the times are pretty much the same either way.)

Yes, it is. I don't have all of the folders you have set to ignore for my antimalware software, maybe that is it?

Even without the exclusions, I never saw it this slow while typing. The initial analysis may be slower, but while typing we shouldn't be reading much from disk (we're analyzing the in-memory overlay). I think it could be worth doing some profiling with the CPU profiler to see exactly what is taking the time when you're typing like this.

You can use dart.analyzerVmServicePort in your workspace settings for this project to enable the VM Service for the analyzer, and then open DevTools CPU profile and connect to it (http://localhost:x where x is the port you picked), start profiling, type a little in the editor, then review (or share) the profile.

@bwilkerson

@bwilkerson I managed to repro this in a test (test/lsp/diagnostic_test.dart) that I've included below and I now understand what's happening, but I'm not sure what the right fix is.

What happens is when you type export 'Foo.dart';, VS Code requests code actions and we include a fix for "Create file 'Foo.dart';". Inside the implementation of the CreateFile fix we create a DartFileEditBuilder to create the edit that will insert // TODO Implement this library.. Inside DartFileEditBuilder, we call getResolvedUnit() which triggers analysis of this non-existant file Foo.dart. Analysis of that file produces a diagnostic when the file_names lint is enabled, and that is then forwarded to the client (because we listen for all analysis results).

Some thoughts:

Should we be computing any diagnostics and/or lints for files that don't exist? (I'm surprised we get as far as running the lints!)
Should we be sending diagnostics for any files that don't exist? (I feel like we shouldn't, but I also worry whether we might leave stale diagnostics if we just skip this.. so we'd probably want to send an empty set if the file doesn't exist instead, but it feels clunky)
Should getResolvedUnit even return a ResolvedUnitResult for non-existent files?

Test that repros (probably not the right place for a test, but where I started when trying to repro outside of VS Code):

 /// Verify that computation of the QuickFix for "Create file 'Foo.dart'" does
  /// not trigger lint warnings for that file because the CreateFile fix tries
  /// to get a resolved unit which triggers analysis.
  @soloTest
  Future<void> test_directive_missingFile() async {
    registerLintRules();
    registerBuiltInProducers();
    setSupportedCodeActionKinds([CodeActionKind.QuickFix]);
    newFile(mainFilePath, "export 'Danny.dart';");
    newFile('$testPackageRootPath/lib/analysis_options.yaml', '''
linter:
  rules:
    - file_names
''');

    await initialize();
    var pos = Position(line: 0, character: 0);
    await getCodeActions(mainFileUri, position: pos);
    await pumpEventQueue(times: 5000);

    // We don't expect the lint diagnostic for the filename Danny.dart because
    // it doesn't exist.
    expect(diagnostics.keys, [mainFileUri]);
  }

@scheglov

Should we be computing any diagnostics and/or lints for files that don't exist?

I can't see any reason to do so.

Should we be sending diagnostics for any files that don't exist? (I feel like we shouldn't, but I also worry whether we might leave stale diagnostics if we just skip this.. so we'd probably want to send an empty set if the file doesn't exist instead, but it feels clunky)

If we can detect that a file has been deleted (and I think we can) then we ought to be able to send an empty list of diagnostics at that point rather than every time we re-discover that the file no longer exists.

Should getResolvedUnit even return a ResolvedUnitResult for non-existent files?

If it doesn't then we'd have to fix DartFileEditBuilder to work when there's no ResolvedUnitResult. Or find some other way to allow us to build the edit used to create the file with some given content.

@scheglov

@bwilkerson

@bwilkerson

Should getResolvedUnit even return a ResolvedUnitResult for non-existent files?

If it doesn't then we'd have to fix DartFileEditBuilder to work when there's no ResolvedUnitResult. Or find some other way to allow us to build the edit used to create the file with some given content

Good point, probably we shouldn't change that then.

I can think of a few possible fixes:

Change the file_names lint to never produce a diagnostic for non-existent files (I suspect it's probably the only one that does, but it's not guaranteed now and forever)
Skip running lints for non-existent files (probably in LibraryAnalyzer._computeLints()?)
Change the analysis server to just send an empty set whenever it's sending diagnostics for a non-existent file (probably in NotificationManager.sendAnalysisErrors()?)

I felt like the last one would be best, because we don't have to worry about skipping sending diagnostics that might result in stale data (eg. if a file is deleted and then we never send an event to clear them on the client). However, NotificationManager.sendAnalysisErrors only works with paths so doesn't have the ResolvedUnitResult at this point (and in fact, there might not be one, since we can produce diagnostics for non-Dart files).

Any thoughts/opinions on the best way to fix?

@scheglov

The error that you imported a not existing file is not in the not existing file, so we don't need its ResolvedUnitResult.

That's true, but we produce a fix ("Create file 'foo.dart'") which has some default content in it, so we use the DartFileEditBuilder with the ResolvedUnitResult for the non-existent file (we also produce the file_names diagnostic against it.. there are two diagnostics produced here - the one that you're importing a missing file, and one that the missing file that we analyzed has a name that violates the lint).

For the analyzer it is not very useful to report diagnostics for a not existing file.
And this will also fix the observed issue with sending these diagnostics by DAS.

We don't have to use DartFileEditBuilder, AFAIK nothing in the protocol requires this, all we need is a SourceFileEdit. It looks that we have addGenericFileEdit that might be useful.

Change the file_names lint ...

That does seem a little too special-case.

Skip running lints for non-existent files ...

That's nice and general, and might improve performance by skipping some work, though it presumably doesn't happen often, so meh.

Change the analysis server to just send an empty set whenever it's sending diagnostics for a non-existent file ...

I think that one would also be fine.

So I'm OK with either of the latter two.

I'm assuming that for both of these latter two cases we would ask the FileState for existence information because asking the resource manager would be too inefficient.

We don't have to use DartFileEditBuilder, AFAIK nothing in the protocol requires this, all we need is a SourceFileEdit. It looks that we have addGenericFileEdit that might be useful.

That's true, though changing getResolvedUnit to not return a ResolvedUnitResult for a non-existing file is probably a breaking change? (if we did this, we should probably deprecate/remove exists and update any code using it to expect null instead?).

I'll try implementing (2) for now, since I agree it's better to not produce them (or run the code) than just filter them out lately, and I guess we must already be handling flushing errors for deleted files reliably (and not relying on something triggering analysis of those now-deleted files).

I'm assuming that for both of these latter two cases we would ask the FileState for existence information because asking the resource manager would be too inefficient.

Yep, I think so (because we also need it to be consistent and I'm not sure if resource manager would be guaranteed to be).

I've opened https://dart-review.googlesource.com/c/sdk/+/398881.

CreateField does not have to use addDartFileEdit, so I don't understand the comment about getResolvedUnit. We don't have to invoke it, right?

CreateFile does not have to use addDartFileEdit

Yeah, we could definitely change this fix to use a generic file edit builder and/or handle getResolvedUnit() returning null for non-existent files. We could also update all places that call getResolvedUnit and check exists to instead expect null. But if there is external code calling getResolvedUnit() and checking exists (or at least, handling non-null with exists=false differently to null), I think we should break them?

If you think we should do this (effectively making exists on ResolvedUnitResult redundant because it would always be true if I understand correctly?), I could try making the change to see if anything fails (and then we can decide how to release it if it's considered breaking)?

Which getResolvedUnit are we discussing here? The one from AnalysisSession? Or from AnalysisServer?

The AnalysisSession one already returns SomeResolvedUnitResult that can be many results, not necessary ResolvedUnitResult.

Analyzing a not existing file is not very useful, so we could make it NotExistingPathResult if we want. Not sure how useful it is too though.

[analyzer] Don't run lints for files that don't exist

Fixed by dart-lang/sdk@bb4850b - ships in a future SDK release.

Which getResolvedUnit are we discussing here? The one from AnalysisSession? Or from AnalysisServer?

Oh, I was looking at AnalysisSession but I see my comment about null was slightly incorrect because I was looking at code that did session?.getResolvedUnit(path) (so the null only came from the lack of a session, not the return value).

There aren't many places that check exists on ResolvedUnitResult:

So we could perhaps use a NotExistingPathResult and update those places. However...

I thought of another place currently using addDartFileEdit for potentially-missing files, and that's the "Move Top Level to File" refactor. You can choose either an existing file or a new one. Part of what it does is add imports, so I don't think we could use a generic file builder here.

I'm not sure what scale of changes it would need if a non-existent file no longer had a ResolvedUnitResult though - there do seem to be quite a few references to it in the edit builder (accessing things like path, file, content, libraryElement, lineInfo). I suspect most of them are trivial changes to handle it missing, but whether it's worth adding all those conditions in order to change this is less clear - I'm happy to try it out if you think it would be better.

I don't know what is the definitely good way here.

It was only a theoretical view, analyzing not existing file might be not useful.
However we see that it might be useful after all.

So, we could turn around, and think if analyzing such file is detrimental.
So far, what issues with this we know?
Only this one?

Yeah, this was the only issue I know of where analyzing missing files was causing a problem (because the lint is based only on the name and doesn't require any actual file/content to produce a diagnostic).

(This issue is fixed by dart-lang/sdk@bb4850b, so there are now no known issues - to me - as a result of this)

Navigation Menu

Repository navigation

File with lower_case_with_underscores identifier message is persisting. #5343

General info

Project info

Process info

1 remaining item

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

File with lower_case_with_underscores identifier message is persisting. #5343

Description

Activity

DanTup commented on Nov 19, 2024

FMorschel commented on Nov 19, 2024

DanTup commented on Nov 19, 2024

FMorschel commented on Nov 19, 2024

General info

Project info

Process info

DanTup commented on Nov 26, 2024

FMorschel commented on Nov 26, 2024

DanTup commented on Nov 26, 2024

DanTup commented on Nov 26, 2024

bwilkerson commented on Dec 2, 2024

1 remaining item

DanTup commented on Dec 3, 2024

scheglov commented on Dec 3, 2024

bwilkerson commented on Dec 4, 2024

DanTup commented on Dec 4, 2024

DanTup commented on Dec 4, 2024

scheglov commented on Dec 4, 2024

DanTup commented on Dec 4, 2024

scheglov commented on Dec 4, 2024

DanTup commented on Dec 4, 2024

DanTup commented on Dec 5, 2024

scheglov commented on Dec 5, 2024

DanTup commented on Dec 5, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions