Consider detecting issues in blacklisted code too (only for newly introduced issues)

[sylvestre]

Here https://bugzilla.mozilla.org/show_bug.cgi?id=1405554#c18, it failed because of

[task 2018-02-08T17:19:11.582Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
[task 2018-02-08T17:19:35.807Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/tools/mach_commands.py:8:1 | 'os' imported but unused (F401)
[task 2018-02-08T17:19:35.807Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/tools/mach_commands.py:9:1 | 'stat' imported but unused (F401)
[task 2018-02-08T17:19:35.807Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/tools/mach_commands.py:10:1 | 'platform' imported but unused (F401)
[task 2018-02-08T17:19:35.808Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/tools/mach_commands.py:11:1 | 're' imported but unused (F401)
[task 2018-02-08T17:19:35.808Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/tools/mach_commands.py:167:1 | expected 2 blank lines, found 1 (E302)

I guess there is a different way to call flake8 @ahal any idea?

[marco-c]

I think our static analysis job might have missed this because we are only considering linting failure on modified lines. Probably here the patch somehow removed usage of those modules, but didn't change the lines where they are imported, so we ignored the linting errors on those lines.

I think we should consider linting failures on all lines and not only modified lines. It makes sense for the clang-based analyses to only consider modified lines, but not for mach lint (since this one already runs as part of automation, so there are no pre-existing issues that have to be ignored).

[sylvestre]

OK, we should probably start working on what we discussed at FOSDEM (like @jvillard is doing).

1) do a build without the patch 2) do a build with the patch 3) diff the two results

Thanks to that, we would avoid our heuristic to know what is impacted or not. It would increase the delay but we have room for improvements here.

@La0 @marco-c @ahal @Standard8 Any opinion on that?

[marco-c]

I think for mach lint this probably doesn't apply.

For clang, we know there are already problems in the code, and we only want to avoid the introduction of new ones.

For mach lint, the existing code is "clean", so we want to show any error the developer introduces, no matter where it is.

[sylvestre]

For mach lint, the existing code is "clean", so we want to show any error the developer introduces, no matter where it is.

Yes and no. I would like us to start also showing linting errors/warnings outside of the whitelist (to avoid making things worst).

[ahal]

I think we should be linting the entire file, not just the diff of what changed. I also disagree about linting outside of the whitelists/blacklists. What I like about the current system is that module owners can opt-in/opt-out of whatever linters they choose.

While in some cases I agree it's best to enforce a common style across the whole tree (eslint/flake8), there are also very specialized linters that we don't want to run everywhere. The py2/py3 compatibility linters are prime examples of this. We might have modules that support python3 and we know they'll never support python2 (or vice versa). So running the py2 linter on that module will cause false issues to be posted. Same with running the wpt linter on non wpt test files.

I think if we do want to run a linter everywhere, we just need to bite the bullet and actually fix the previously existing errors. Luckily in the case of flake8, we can use the autopep8 module (and there's a bug to hook this up to --fix).

[marco-c]

Wait, to be clear, I'm not arguing for running the linter on everything, I only think we should run it on all files modified by the patch and not only on modified lines, otherwise we are not going to notify about issues that are then going to make the lint job fail.

[ahal]

Yes, we are still talking about only linting files that were touched. Most of the linters are configured to only run a certain subset of the tree. For example, flake8 only runs on this whitelist: https://searchfox.org/mozilla-central/source/tools/lint/flake8.yml#4

Sometimes (like in the case of flake8), we only do this to avoid all the previously existing failures. Sylvestre is suggesting we run the reviewbot on files even if they aren't configured to run with that particular linter (and in order to not be super annoying, the reviewbot would need to only check diffed lines). This way, we nudge people to fix lint errors across the whole tree as they change it.

But my counter point is that in other cases, we intentionally include/exclude directories because we explicitly don't want the linter running there. So imo, it's better to not lint files that aren't configured to run with the linter because: A) The exclusion might be intentional B) It forces us to lint diffs which results in issues like this one

The alternative is to take the approach we did for eslint, incrementally (and heroically) fix every issue and enable the linter as we go.

[Standard8]

But my counter point is that in other cases, we intentionally include/exclude directories because we explicitly don't want the linter running there.

Just to add to this, there's one area where the module owner has explicitly said to me that they don't want ESLint there at the moment - certainly not in the current configuration, so we wouldn't want those files showing up via reviewbot (I do have plans/hopes for the future that should get it so we can enable it there).

[Standard8]

One issue we do have at the moment with ESLint, is that we're not catching changes caused by configuration/dependent files. On the mozilla-central side this is bug 1373368.

The main issues for ESLint, is that we don't pick up failures when:

• Changes to the .eslintrc.js files raise issues in the directory.
• Changes to tools/lint/eslint/* raise issues outside of the directory.
• Changes to files that eslint-plugin-mozilla parses cause issues in the tree.

On that last point, I heard of someone today who changed Services.jsm which then caused linting errors that were only picked up once it landed on autoland (as that's currently the only place we do a full scan in these cases).

Not sure that's directly related to this issue, but we might need to cover it somewhere.

[sylvestre]

Similar issue: https://bugzilla.mozilla.org/show_bug.cgi?id=1437226

[marco-c]

To fix the problem, I think we just need to make our bot lint entire files and not only modified lines. Given that the issue has morphed into not introducing new errors, I've opened #834 for the original problem.

Regarding linting excluded files/directories, if we think we are moving towards linting everything at some point, then ensuring we don't introduce new errors sounds good to me. If we fail the job only when new errors are introduced, we won't annoy people about pre-existing issues. But I think fixing this issue by linting entire files rather than modified lines should happen regardless of the outcome of this discussion.

B) It forces us to lint diffs which results in issues like this one

Instead of linting diffs, we could lint entire files twice (before and after the patch) and ensure there isn't any error that is in after the patch and not in before the patch.

[sylvestre]

Instead of linting diffs, we could lint entire files twice (before and after the patch) and ensure there isn't any error that is in after the patch and not in before the patch.

I think this is probably the best approach (it is what facebook is using). We should give it a try to see if we would have catch these issues;

[marco-c]

I think this is probably the best approach (it is what facebook is using). We should give it a try to see if we would have catch these issues;

I think the issues were not caught because of #834 (which is now fixed). The bot now catches all linting issues in non-blacklisted files (as the linting job does), so these issues would have been caught now.

Now we should decide whether we want to start showing issues in blacklisted files. I think it would make sense to only show new issues by using this approach, but I suppose it could annoy some developers (since they have marked their directory as "ignore").