No ErrorCode for _LintCode.invariant_boolean in /src/engine/src/tools/licenses/lib/licenses.dart #28803

Open
eseidelGoogle opened this Issue Feb 17, 2017 · 17 comments

Projects

None yet

5 participants

@eseidelGoogle

Exception from analysis server (running from VSCode / Dart Code)

What I was doing

Opened https://github.com/flutter/engine/tree/master/tools/licenses with the latest dart dev in visual studio code.

Versions

  • Dart SDK 1.22.0-dev.5.0
  • Visual Studio Code 1.9.1
  • Dart Code 0.15.1

Analyzer Info

The analyzer was launched with the arguments:

--client-id=DanTup.dart-code
--client-version=0.15.1

Exception

Analysis failed: /src/engine/src/tools/licenses/lib/licenses.dart

Bad state: No ErrorCode for _LintCode.invariant_boolean in /src/engine/src/tools/licenses/lib/licenses.dart
#0      AnalysisDriver._getErrorsFromSerialized.<anonymous closure> (package:analyzer/src/dart/analysis/driver.dart:888)
#1      MappedListIterable.elementAt (dart:_internal/iterable.dart:413)
#2      ListIterable.toList (dart:_internal/iterable.dart:219)
#3      AnalysisDriver._getErrorsFromSerialized (package:analyzer/src/dart/analysis/driver.dart:892)
#4      AnalysisDriver._getAnalysisResultFromBytes (package:analyzer/src/dart/analysis/driver.dart:859)
#5      AnalysisDriver._computeAnalysisResult.<anonymous closure> (package:analyzer/src/dart/analysis/driver.dart:711)
#6      PerformanceLog.run (package:analyzer/src/dart/analysis/driver.dart:1479)
#7      AnalysisDriver._computeAnalysisResult (package:analyzer/src/dart/analysis/driver.dart:674)
#8      AnalysisDriver._performWork.<_performWork_async_body> (package:analyzer/src/dart/analysis/driver.dart:1033)
#9      Future.Future.microtask.<anonymous closure> (dart:async/future.dart:184)
#10     _rootRun (dart:async/zone.dart:1146)
#11     _CustomZone.run (dart:async/zone.dart:1026)
#12     _CustomZone.runGuarded (dart:async/zone.dart:924)
#13     _CustomZone.bindCallback.<anonymous closure> (dart:async/zone.dart:951)
#14     _rootRun (dart:async/zone.dart:1150)
#15     _CustomZone.run (dart:async/zone.dart:1026)
#16     _CustomZone.runGuarded (dart:async/zone.dart:924)
#17     _CustomZone.bindCallback.<anonymous closure> (dart:async/zone.dart:951)
#18     _microtaskLoop (dart:async/schedule_microtask.dart:41)
#19     _startMicrotaskLoop (dart:async/schedule_microtask.dart:50)
#20     _Timer._runTimers (dart:isolate-patch/timer_impl.dart:394)
#21     _Timer._handleMessage (dart:isolate-patch/timer_impl.dart:414)
#22     _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:148)
@eseidelGoogle

dartanalyzer doesn't crash at the command line for what that's worth:

eseidel@eseidel-macbookpro /src/engine/src/tools/licenses %dartanalyzer .                                                               [/src/engine/src/tools/licenses]
Analyzing [.]...
[lint] Conditions should not unconditionally evaluate to "TRUE" or to "FALSE" (/src/engine/src/tools/licenses/lib/licenses.dart, line 516, col 3)
[lint] Conditions should not unconditionally evaluate to "TRUE" or to "FALSE" verify: results.length == 1. (/src/engine/src/tools/licenses/lib/licenses.dart, line 845, col 9)
[lint] Conditions should not unconditionally evaluate to "TRUE" or to "FALSE" (/src/engine/src/tools/licenses/lib/licenses.dart, line 516, col 3)
[lint] Conditions should not unconditionally evaluate to "TRUE" or to "FALSE" verify: results.length == 1. (/src/engine/src/tools/licenses/lib/licenses.dart, line 845, col 9)
[lint] Specify type annotations. (/src/engine/src/tools/licenses/lib/main.dart, line 1041, col 43)
5 lints found.
@eseidelGoogle

Hmm. line 516 is a while(true) which is a valid construct. :)

@eseidelGoogle

Presumably I should have filed this at:
https://github.com/dart-lang/linter @pq?

@pq
Member
pq commented Feb 17, 2017

Interesting. This looks like a version skew issue -- invariant_boolean became invariant_booleans (for consistency) and it looks an old version of the linter leaked into the wild. I need to track down how that happened... In the meantime, @scheglov : could you advise on this exception? Is there an easy work-around?

@pq
Member
pq commented Feb 17, 2017

Hmmm, actually @eseidelGoogle, if I'm not misreading, I notice you're on an old SDK? As of yesterday, we're on 1.23.0-dev.0.0. It would be good to know if the issue persists with the latest.

@pq
Member
pq commented Feb 17, 2017

With the latest everything, I couldn't repro the exception. 👍

screen shot 2017-02-17 at 5 57 20 am

screen shot 2017-02-17 at 5 58 14 am

screen shot 2017-02-17 at 5 58 52 am

@eseidelGoogle : maybe retry with the latest?

As for the value of the lint, we should take that up over on the linter and loop in the author (@alexeieleusis).

@scheglov
Contributor

@pq No, if lint announces that it will generate errors of one kind, and generated of another one, there is nothing we can do on the analysis driver level. Even if we remove the analysis cache, when the file is reanalyzed, the same problem will happen again.

@bwilkerson
Member

... if lint announces that it will generate errors of one kind, and generated of another one ...

There's another possible explanation: it's possible that an older version of the linter created the lint (with the then-correct name of invariant_boolean), the lint was written to the summary file, then the linter was updated and when we tried to resynthesize the error we couldn't find the rule. The data in the cache appeared to be valid because we didn't increment the version number when the lint rule's name changed.

One possible solution would be to change the code so that if we cannot resynthesize a given summary file, we delete it from the cache and then proceed as if the original cache probe hadn't found a file. This seems like a safe fallback, and preferable to failing analysis.

@eseidelGoogle
eseidelGoogle commented Feb 17, 2017 edited

I've upgraded my dart sdk to 1.23.0-dev.0.0 and yes the analyzer no longer crashes in visual code.

One thing that was interesting here was that it only crashed inside visual code and not from the command linedartanalyze for whatever reason.

@scheglov
Contributor
scheglov commented Feb 17, 2017 edited

dartanalyze was switched to use the new analysis driver only yesterday, and is not included into 1.22.0-dev.5.0 or even 1.23.0-dev.0.0 yet.

@pq
Member
pq commented Feb 17, 2017

One possible solution would be to change the code so that if we cannot resynthesize a given summary file, we delete it from the cache and then proceed as if the original cache probe hadn't found a file. This seems like a safe fallback, and preferable to failing analysis.

👍 SGTM

@alexeieleusis
Member

Apologies for the confusion with the lint. It needs some cleanup, it was splitted at some point into two, one for expressions formed entirely of literals and the other for cases that required a deeper analysis. They both get reported with very similar messages and it is a mistake, it is working as intended though, IMO since this type of condition is exceptional it is worth an annotation to ignore it.

@bwilkerson
Member

... since this type of condition is exceptional it is worth an annotation to ignore it.

It might not be as exceptional as you think. A recent search of a large code base turned up over 700 files containing "while (true)", many of which had multiple uses of this pattern.

@pq
Member
pq commented Feb 22, 2017

No sweat, and thanks @alexeieleusis !

Given the frequency of while(true), how do you feel about permitting that case?

@alexeieleusis
Member

I think that is exactly what we want to lint. I am surprised it is so popular, I would be interested in going through some of those occurrences if possible and see if there is something that can help discarding those usages. Now if this is something that is common in domains like parsers, and things heavily optimized, probably those codebases should not use it.

This rule was conceived (I saw the idea in SonarQube) precisely for those cases where while debugging you change a condition to always be true or false and forget to revert that change, directly using true is the straightforward way to do it, but I guess that with some compilers it gets removed and that's why using something like 1 > 0 has to be used (which we also lint).

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