Skip to content

Excluding files from static analysis doesn't appear to stop errors from being generated in those files #28754

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

Closed
DanTup opened this issue Feb 12, 2017 · 12 comments
Labels
legacy-area-analyzer Use area-devexp instead. P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@DanTup
Copy link
Collaborator

DanTup commented Feb 12, 2017

I don't know if this is expected; but I'm trying out the built_value package hoping to get JSON support. I found the generated code didn't seem to "compile" in strong mode (@davidmorgan - is this expected? I can see commits suggesting strong mode should work; there is generated code like var key;).

I tried adding the generated file to the exclude list in analysis_options.yaml but even after restarting the Analyzer it still reports errors on in those files.

Excluded files

I don't know if this is expected (nor do I know if the generated code is intended to be strong-mode compliant) but I figured I should ask!

@bwilkerson
Copy link
Member

I don't know what's causing the issue, but I can suggest a couple of possibilities.

First, it's possible that the analysis server failed to respond correctly to the exclusion request and continued to report errors and / or failed to tell the client to flush information for the newly excluded files.

Second, I know that we are still refining the definition of strong mode. One implication of this is that the built_value package might well have been made strong mode clean at some earlier point and then had the definition of strong mode change in such a way that it was no longer strong mode clean. Another implication is that different versions of analyzer will report different errors in strong mode because they're using different definitions of what "strong mode" means. Were you using a different version of analyzer between the time you disabled strong mode and when you re-enabled it?

@DanTup
Copy link
Collaborator Author

DanTup commented Feb 12, 2017

it's possible that the analysis server failed to respond correctly to the exclusion request and continued to report errors and / or failed to tell the client to flush information for the newly excluded files.

I'm setting the exclusion in analysis_options.yaml so there wasn't a specific request to the analyzer for this. I also reloaded everything (resulting in an empty errors list and a new analyzer being created) to eliminate that possibility.

One implication of this is that the built_value package might well have been made strong mode clean

I discovered the issue here wasn't just strong mode, it was implicit dynamics. I've raised a request for the generated code to support that; but at least that bit makes sense now.

Were you using a different version of analyzer between the time you disabled strong mode and when you re-enabled it?

I don't believe so, I've been using 1.21.1 almost the entire day (I did some testing earlier, but that was before I really started any of this work).

I think the errors coming/going probably isn't that interesting for now (I had other errors that might've affected those appearing; I can raise an issue if I can reliably repro an issue there); this case was more about the exclude not working. I'll see if I can get some better info...

@DanTup
Copy link
Collaborator Author

DanTup commented Feb 12, 2017

Ok; I think I know what's happening here.

When I open a project with ignored files, I do not get errors for them (this is expected). If I then open a file in the IDE (which will send setPriorityFiles with that file) then the errors appear (I don't know if that's expected, but it seems reasonable to me). When I close the file (which will send setPriorityFiles with the file removed) the error does not get removed (this is probably not expected).

So, I think maybe ignored files should have their errors flushed when they are removed from priority files (assuming the behaviour of showing errors while they're open is correct; otherwise not showing them would also solve the issue).

@zoechi
Copy link
Contributor

zoechi commented Feb 13, 2017

Might be related to #26212

@vsmenon vsmenon added the legacy-area-analyzer Use area-devexp instead. label Feb 14, 2017
@brianegan
Copy link

I've been experiencing this same issue -- I've added an exclude rule for a directory, using analyzer 1.22.0, and it's not picking it up :(

Checked that my .analysis_options is correctly formed, tried wildcards (**) and specific files, even copied rules directly from major projects, but exclude didn't work for me :(

@davidmorgan
Copy link
Contributor

FWIW -- built_value will try to generate code as "analyzer friendly" as possible. (I will take a look to see what can be done about the current warnings).

Good support for excluding warnings for generated files is important.

This doesn't quite end the story, though. There is a more complex issue because built_value works by generating an implementation of your abstract class. If your abstract class implements an interface that built_value doesn't know/care about ... then the generated class has an analyzer warning that you do want to see, because you need to fix it yourself.

e.g. if you implement "HasFoo", with a "foo" property, but don't write that "foo" property yourself; then the class built_value generates also implements HasFoo, is not abstract, and ideally you should get a warning saying that your override is incorrect.

Unfortunately I don't see any way to fix this besides built_value doing the check itself and failing the generation step if there are "left over" abstract methods that you need to implement yourself. This has the pretty major downside that what you're used to seeing as an analyzer warning is delivered via an entirely different path.

@DanTup
Copy link
Collaborator Author

DanTup commented Feb 20, 2017

Unfortunately I don't see any way to fix this besides built_value doing the check itself and failing the generation step if there are "left over" abstract methods that you need to implement yourself.

It's out of scope for this case, but if generating code this way is going to be a common case, I think there should be some ability to route messages back to the analyzer. This isn't just for if a user decides to ignore warnings/errors in the generated code; often the errors in that code do not accurately describe the problem, whereas the author of the code-gen would be able to?

@bwilkerson
Copy link
Member

To perpetuate the side-trail...

As it happens, I'm in the process of implementing support for plugins to the analyzer / analysis server that would allow packages like built_value to contribute to analysis. There isn't a way for plugins to remove or replace errors that analyzer generates, but they could generate their own, and they will be able to contribute quick fixes (for their own errors or those generated by analyzer).

If I don't get sidetracked, I'm hoping to have an initial version within a couple of months.

@DanTup
Copy link
Collaborator Author

DanTup commented Feb 20, 2017

@bwilkerson Sounds good! Is there a GH Issue relating to this we can follow for updates? (just for curiosity, not to continually ask if it's finished yet.. probably).

@bwilkerson
Copy link
Member

Not that I'm aware of.

@zoechi
Copy link
Contributor

zoechi commented Aug 3, 2018

Dup of #25551 ?

@DanTup
Copy link
Collaborator Author

DanTup commented Aug 3, 2018

Yeah, seems like it to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
legacy-area-analyzer Use area-devexp instead. P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

6 participants