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

The analysis exclude path base is from the source directory, not the package root #31344

Closed
yyoon opened this Issue Nov 9, 2017 · 12 comments

Comments

Projects
None yet
6 participants
@yyoon
Contributor

yyoon commented Nov 9, 2017

(related to #31343)

The documentation suggests that the exclude file paths are relative to the package root (seeing that lib/client/piratesapi.dart and test/*_example.dart are in the examples). However, we've noticed that the exclude pattern works only if the paths are relative to the source directory (i.e. lib or test).

[This works]

analyzer:
  exclude:
    - 'testdata/directives_test/after.dart'
    - 'testdata/directives_test/before.dart'
    - 'testdata/double_quotes_test/after.dart'
    - 'testdata/double_quotes_test/before.dart'

[This doesn't work]

analyzer:
  exclude:
    - 'test/testdata/directives_test/after.dart'
    - 'test/testdata/directives_test/before.dart'
    - 'test/testdata/double_quotes_test/after.dart'
    - 'test/testdata/double_quotes_test/before.dart'

This was happening in a fuchsia dart package, in case it matters.

cc: @pylaligand @zanderso @bwilkerson

@MichaelRFairhurst

This comment has been minimized.

Show comment
Hide comment
@MichaelRFairhurst

MichaelRFairhurst Feb 13, 2018

Contributor

I reproed this just now...looks like there's a discrepancy between what the cli analyzer expects vs server. Server handles this correctly, and cli doesn't.

Will begin digging in further!

Contributor

MichaelRFairhurst commented Feb 13, 2018

I reproed this just now...looks like there's a discrepancy between what the cli analyzer expects vs server. Server handles this correctly, and cli doesn't.

Will begin digging in further!

@MichaelRFairhurst

This comment has been minimized.

Show comment
Hide comment
@MichaelRFairhurst

MichaelRFairhurst Feb 15, 2018

Contributor

Note: this is actually an issue of where a path is passed in to dartanalyzer other than the analysis_options path. I'm assuming this is:

dartanalyzer test

instead of

dartanalyzer .

it may not be super high priority for us since it has a workaround and is a bit nastier than I expected to fix, but I'm still looking into it.

Contributor

MichaelRFairhurst commented Feb 15, 2018

Note: this is actually an issue of where a path is passed in to dartanalyzer other than the analysis_options path. I'm assuming this is:

dartanalyzer test

instead of

dartanalyzer .

it may not be super high priority for us since it has a workaround and is a bit nastier than I expected to fix, but I'm still looking into it.

@pylaligand

This comment has been minimized.

Show comment
Hide comment
@pylaligand

pylaligand Feb 15, 2018

Contributor

We are experiencing this problem with the analysis server.

Contributor

pylaligand commented Feb 15, 2018

We are experiencing this problem with the analysis server.

@bwilkerson

This comment has been minimized.

Show comment
Hide comment
@bwilkerson

bwilkerson Feb 15, 2018

Member

Where is the analysis options file relative to the directory you're opening in the IDE?

Member

bwilkerson commented Feb 15, 2018

Where is the analysis options file relative to the directory you're opening in the IDE?

@pylaligand

This comment has been minimized.

Show comment
Hide comment
@pylaligand

pylaligand Feb 15, 2018

Contributor

Here's the layout:

$root/
  $some/
    $path/
      analysis_options.yaml
      lib/
      test/
        testdata/
Contributor

pylaligand commented Feb 15, 2018

Here's the layout:

$root/
  $some/
    $path/
      analysis_options.yaml
      lib/
      test/
        testdata/
@MichaelRFairhurst

This comment has been minimized.

Show comment
Hide comment
@MichaelRFairhurst

MichaelRFairhurst Feb 19, 2018

Contributor

I think, and please correct me if I'm wrong, you're experiencing the opposite issue with the analysis server.

If I load dartfmt_extras in intelliJ, the excludes don't work because they don't have the 'test/' prefix. Adding the test/ prefix fixes it -- this is also what the spec says should be done.

However, then if you run dartanalyzer test/ then the excludes once again don't work, until you remove the test/ prefix. Which seems to be what someone last did.

For forwards compatibility with the fixes incoming, and to get it working with the current correct behavior of dartanalyzer, I'd recommend listing both:

analyzer:
  exclude:
    # keep these for now while the commandline dartanalyzer has a bug
    - 'testdata/directives_test/after.dart'
    - 'testdata/directives_test/before.dart'
    - 'testdata/double_quotes_test/after.dart'
    - 'testdata/double_quotes_test/before.dart'
    # this is the correct way to do it, and works in intellij
    - 'test/testdata/**.dart'
Contributor

MichaelRFairhurst commented Feb 19, 2018

I think, and please correct me if I'm wrong, you're experiencing the opposite issue with the analysis server.

If I load dartfmt_extras in intelliJ, the excludes don't work because they don't have the 'test/' prefix. Adding the test/ prefix fixes it -- this is also what the spec says should be done.

However, then if you run dartanalyzer test/ then the excludes once again don't work, until you remove the test/ prefix. Which seems to be what someone last did.

For forwards compatibility with the fixes incoming, and to get it working with the current correct behavior of dartanalyzer, I'd recommend listing both:

analyzer:
  exclude:
    # keep these for now while the commandline dartanalyzer has a bug
    - 'testdata/directives_test/after.dart'
    - 'testdata/directives_test/before.dart'
    - 'testdata/double_quotes_test/after.dart'
    - 'testdata/double_quotes_test/before.dart'
    # this is the correct way to do it, and works in intellij
    - 'test/testdata/**.dart'
@pylaligand

This comment has been minimized.

Show comment
Hide comment
@pylaligand

pylaligand Feb 21, 2018

Contributor

You are right, it was the other way around.

However if I load dartfmt_extras in Atom with a very recent Dart SDK, I'm still seeing some errors/warnings.

Contributor

pylaligand commented Feb 21, 2018

You are right, it was the other way around.

However if I load dartfmt_extras in Atom with a very recent Dart SDK, I'm still seeing some errors/warnings.

@MichaelRFairhurst

This comment has been minimized.

Show comment
Hide comment
@MichaelRFairhurst

MichaelRFairhurst Feb 21, 2018

Contributor

@pylaligand I think we're getting to the bottom of our confusion, awesome.

Did the changes I suggested (to analysis_options.yaml) fix the issues in Atom for you?

Contributor

MichaelRFairhurst commented Feb 21, 2018

@pylaligand I think we're getting to the bottom of our confusion, awesome.

Did the changes I suggested (to analysis_options.yaml) fix the issues in Atom for you?

@pylaligand

This comment has been minimized.

Show comment
Hide comment
@pylaligand

pylaligand Feb 21, 2018

Contributor

Here's the state of the options file: https://fuchsia-review.googlesource.com/c/topaz/+/125459/1/tools/dartfmt_extras/analysis_options.yaml

I'm still seeing errors in Atom. Note that they start showing up the moment I open an excluded file.

Contributor

pylaligand commented Feb 21, 2018

Here's the state of the options file: https://fuchsia-review.googlesource.com/c/topaz/+/125459/1/tools/dartfmt_extras/analysis_options.yaml

I'm still seeing errors in Atom. Note that they start showing up the moment I open an excluded file.

@bwilkerson

This comment has been minimized.

Show comment
Hide comment
@bwilkerson

bwilkerson Feb 21, 2018

Member

I'm still seeing errors in Atom. Note that they start showing up the moment I open an excluded file.

Yes, that's a known issue. Originally, the analysis server would not show any information for excluded files: not errors, highlighting, navigation, code completion, not anything. But our users expected that they could navigate, etc. in an excluded file if they opened it, so we "fixed" it by sending back information about excluded files if (and only if) they are opened in the client.

For the most part, this is innocuous because clients discard all of this information when the file is closed. Except for errors, which most clients keep until server tells them to update the list. What I think we really need is to introduce something to the API so that clients will know to discard errors for excluded files when the file is closed (and update clients to understand it). It just hasn't yet been high enough priority to get done.

Member

bwilkerson commented Feb 21, 2018

I'm still seeing errors in Atom. Note that they start showing up the moment I open an excluded file.

Yes, that's a known issue. Originally, the analysis server would not show any information for excluded files: not errors, highlighting, navigation, code completion, not anything. But our users expected that they could navigate, etc. in an excluded file if they opened it, so we "fixed" it by sending back information about excluded files if (and only if) they are opened in the client.

For the most part, this is innocuous because clients discard all of this information when the file is closed. Except for errors, which most clients keep until server tells them to update the list. What I think we really need is to introduce something to the API so that clients will know to discard errors for excluded files when the file is closed (and update clients to understand it). It just hasn't yet been high enough priority to get done.

@pylaligand

This comment has been minimized.

Show comment
Hide comment
@pylaligand

pylaligand Feb 21, 2018

Contributor

That makes sense and is consistent with what I'm observing. Is there a bug tracking that issue?

Contributor

pylaligand commented Feb 21, 2018

That makes sense and is consistent with what I'm observing. Is there a bug tracking that issue?

dart-bot pushed a commit that referenced this issue Mar 5, 2018

Refactor to efficiently find analysis roots correctly fix #31343 & #3…
…1344.

Move logic out into a ContextCache since we're not interested in caching
at least two if not three things (the options, the options path, and
optionally the builder).

Move some convenience methods around accessing contexts into a mixin.

Regarding some semantic decisions for finding analysis roots:
* treat the analysis_options.yaml file as the analysis root if it exists
* if it doesn't exist, assume the directory passed in is the root
* if custom options are provided, assume they "overwrite" the others
  _in-place_.

Tests to confirm this logic via excludes configurations with & without
the yaml file specified, passing in the root in some cases and
subdirectories in others.

Also change to use PathFinder for handling wildcards in excludes.

Bug: 31343,31344
Change-Id: I400fe30a1ec379f9040f812fc0bd9481d42a13cf
Reviewed-on: https://dart-review.googlesource.com/41570
Commit-Queue: Mike Fairhurst <mfairhurst@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
@pylaligand

This comment has been minimized.

Show comment
Hide comment
@pylaligand

pylaligand Mar 15, 2018

Contributor

I verified that this now works. Thanks for fixing!

Contributor

pylaligand commented Mar 15, 2018

I verified that this now works. Thanks for fixing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment