dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.23k stars 1.57k forks source link

Analyzer does not navigate up in the directory tree looking for closest analysis_options.yaml file #56552

Closed gabrielgarciagava closed 2 weeks ago

gabrielgarciagava commented 2 months ago

According to https://dart.dev/tools/analysis#the-analysis-options-file, the analyzer should navigate up in the directory tree looking for the closest analysis_options.yaml file.

If the analyzer can't find an analysis options file at the package root, it walks up the directory tree, looking for one. If no file is available, the analyzer defaults to standard checks.

It was working as described in dart 3.4.0. It is not happening in dart 3.5.0 and 3.5.1.

Do I need to create a minimal sample project to demonstrate it?

dart-github-bot commented 2 months ago

Summary: The analyzer is no longer navigating up the directory tree to find the closest analysis_options.yaml file in Dart 3.5.0 and 3.5.1, which is a regression from Dart 3.4.0. A minimal sample project may be needed to demonstrate the issue.

bwilkerson commented 2 months ago

@pq

gabrielgarciagava commented 2 months ago

Do you mind sharing what is the status of this ticket? Since there is no activity on it for the last few days I'm wondering if it is being worked on or if the priority is low.

We have a big mono repository project that cannot really upgrade to latest flutter because of this issue. I just would like to know what is status so I know how to proceed from our side (implement some workaround or wait for the fix)

keertip commented 2 months ago

Can you give us some details of your monrepo structure. Where are the pubspec.yaml and analsis_options.yaml files located. Which analysis_options file is not getting picked up? What is the directory/folder you are opening in the IDE?

gabrielgarciagava commented 2 months ago
project
|  pkgs
|  |  pkg1
|  |  |  pubspec.yaml
|  |  pkg2
|  |  |  pubspec.yaml
|  |  pkg3
|  |  |  pubspec.yaml
|  |  analysis_options.yaml
|  pubspec.yaml

I am opening the folder project in the IDE. The analysis_options.yaml contains this:

analyzer:
  exclude:
    - "**/*.g.dart"

Prior to dart 3.5.0, and according to the document I shared, everything inside pkg1, pkg2 and pkg3 should consume the analysis_options.yaml that is in the parent folder. I have a lot of .g.dart files in all those internal packages, and after upgrading to dart 3.5.0, all of them are being analyzed (while it should be ignored)

gabrielgarciagava commented 1 month ago

I would like to know what is the status of this ticket.

Does it help if I create some minimal project showing the problem or were you able to reproduce the issue already?

mraleph commented 1 month ago

This seems like a fairly significant regression in behavior, affecting users with large repos. I would think this should be at the very least P1 rather than P2 and be treated accordingly.

pq commented 1 month ago

@gabrielgarciagava: sorry for the inconvenience and the slow response!

Does it help if I create some minimal project showing the problem or were you able to reproduce the issue already?

If that's easy for you to do, it would be greatly appreciated.

Thanks!

gabrielgarciagava commented 3 weeks ago

This should do: https://github.com/gabrielgarciagava/dart_issue_56552

Notice that only the exclusion of .g.dart files is not working. The linter rule prefer_final_locals is actually being properly consumed by pkg1. So it is not the full file that is ignored as I initially stated, it is only the analyzer: exclude: part (that I know of).

In dart 3.4.0 the bad.g.dart file is ignored. In dart 3.5.0 the bad.g.dart file is NOT ignored. In dart 3.5.3 the bad.g.dart file is NOT ignored.

pq commented 3 weeks ago

Thank you!

pq commented 3 weeks ago

Thanks again @gabrielgarciagava! @keertip was able to use your repro locally but we're still trying to get the failure to show up in our tests (https://dart-review.googlesource.com/c/sdk/+/388642)... We'll do some more investigating.

gabrielgarciagava commented 3 weeks ago

Thanks. Taking a quick look to the test assert for multiple context, the only difference I can see in the setup is that in my example the analysis_options.yaml file is located one level deeper. I.e inside "/home/test/lib" instead of "/home/test". No idea if this matters.

gabrielgarciagava commented 3 weeks ago

@keertip Maybe try to change testPackageRootPath to testPackageLibPath in this line to see if the test fails?

The lines with optionsFile: /home/test/analysis_options.yaml should be corrected to optionsFile: /home/test/lib/analysis_options.yaml too I believe (not sure if I understand the testing tools properly).

pq commented 3 weeks ago

Hey @gabrielgarciagava!

Maybe try to change testPackageRootPath to testPackageLibPath in this line to see if the test fails?

Do you mine line 301?:

    newAnalysisOptionsYamlFile(testPackageLibPath, r'''
analyzer:
  exclude:
    - "**/*.g.dart"
''');

Here's the output making that change:

contexts
  /home/test
    packagesFile: /home/test/.dart_tool/package_config.json
    workspace: workspace_0
    analyzedFiles
      /home/test/lib/a.dart
        uri: package:test/a.dart
        analysisOptions_0
        workspacePackage_0_0
      /home/test/lib/b.g.dart
        uri: package:test/b.g.dart
        analysisOptions_0
        workspacePackage_0_0
  /home/test/lib/nested
    packagesFile: /home/test/lib/nested/.dart_tool/package_config.json
    workspace: workspace_1
    analyzedFiles
      /home/test/lib/nested/lib/c.dart
        uri: package:nested/c.dart
        analysisOptions_0
        workspacePackage_1_0
      /home/test/lib/nested/lib/d.g.dart
        uri: package:nested/d.g.dart
        analysisOptions_0
        workspacePackage_1_0
analysisOptions
  analysisOptions_0: /home/test/lib/analysis_options.yaml
workspaces
  workspace_0: PackageConfigWorkspace
    root: /home/test
    pubPackages
      workspacePackage_0_0: PubPackage
        root: /home/test
  workspace_1: PackageConfigWorkspace
    root: /home/test/lib/nested
    pubPackages
      workspacePackage_1_0: PubPackage
        root: /home/test/lib/nested

This is still not quite the layout that you have in your repro but at first blush it does look like it gets at a problem: note that d.g.dart is included in analyzedFiles which it would not if excludes were working as intended.

gabrielgarciagava commented 3 weeks ago

Yes, 301

Hmm, interesting. In the other hard, it looks like the optionsFile is completely gone from both contexts, which makes b.g.dart to also be included.

I mean, it is not the same as my example since in my example the analyzer:exclude: had effect in the upper directory.

gabrielgarciagava commented 3 weeks ago

Ahh, I see, the reason for b.d.dart to be included is because my rule is to exclude **/*g.dart, if I add the files one level deeper, then it mimicks my example. I'm running the test locally.

Changed to

    var workspaceRootPath = '/home';
    var testPackageRootPath = '$workspaceRootPath/test';
    var testPackageLibPath = '$testPackageRootPath/lib';

    newPubspecYamlFile(testPackageRootPath, r'''
name: test
''');

    newSinglePackageConfigJsonFile(
      packagePath: testPackageRootPath,
      name: 'test',
    );

    newAnalysisOptionsYamlFile(testPackageLibPath, r'''
analyzer:
  exclude:
    - "**/*.g.dart"
''');

    var nestedNoYamlPath = '$testPackageLibPath/nestedNoYaml';
    newFile('$nestedNoYamlPath/a.dart', '');
    newFile('$nestedNoYamlPath/b.g.dart', '');

    var nestedPath = '$testPackageLibPath/nested';
    newFile('$nestedPath/lib/c.dart', '');
    newFile('$nestedPath/lib/d.g.dart', '');

    newSinglePackageConfigJsonFile(
      packagePath: nestedPath,
      name: 'nested',
    );
    newPubspecYamlFile(nestedPath, r'''
name: nested
''');

And I get

  Actual: 'contexts\n'
            '  /home/test\n'
            '    packagesFile: /home/test/.dart_tool/package_config.json\n'
            '    workspace: workspace_0\n'
            '    analyzedFiles\n'
            '      /home/test/lib/nestedNoYaml/a.dart\n'
            '        uri: package:test/nestedNoYaml/a.dart\n'
            '        analysisOptions_0\n'
            '        workspacePackage_0_0\n'
            '  /home/test/lib/nested\n'
            '    packagesFile: /home/test/lib/nested/.dart_tool/package_config.json\n'
            '    workspace: workspace_1\n'
            '    analyzedFiles\n'
            '      /home/test/lib/nested/lib/c.dart\n'
            '        uri: package:nested/c.dart\n'
            '        analysisOptions_0\n'
            '        workspacePackage_1_0\n'
            '      /home/test/lib/nested/lib/d.g.dart\n'
            '        uri: package:nested/d.g.dart\n'
            '        analysisOptions_0\n'
            '        workspacePackage_1_0\n'
            'analysisOptions\n'
            '  analysisOptions_0: /home/test/lib/analysis_options.yaml\n'
            'workspaces\n'
            '  workspace_0: PackageConfigWorkspace\n'
            '    root: /home/test\n'
            '    pubPackages\n'
            '      workspacePackage_0_0: PubPackage\n'
            '        root: /home/test\n'
            '  workspace_1: PackageConfigWorkspace\n'
            '    root: /home/test/lib/nested\n'
            '    pubPackages\n'
            '      workspacePackage_1_0: PubPackage\n'
            '        root: /home/test/lib/nested\n'
            ''

And as you can see, the only difference between the 2 folders (nested and nestedNoYaml), is that the "buggy" one contains a pubspec.yaml file.

pq commented 3 weeks ago

And as you can see, the only difference between the 2 folders (nested and nestedNoYaml), is that the "buggy" one contains a pubspec.yaml file.

Yeah. That's a good catch!

@keertip: maybe we can take a look together when you're back?

keertip commented 2 weeks ago

Thanks for the input on the test @gabrielgarciagava . Can see the failure, now to figure out why.