-
Notifications
You must be signed in to change notification settings - Fork 6
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
Issue 336: Underflow not detected for range length field #337
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
isort 4.3.21 on Debian (installed via pip3) seems to be broken and does not detect the third party sources. As a consequence, it formats all sources to remove the blank line between those imports and our local imports. Specifying them in the config fixes the issue and also makes isort more robust in cases where the dependencies are not installed.
jklmnn
previously approved these changes
Jul 10, 2020
treiher
requested changes
Jul 10, 2020
Codecov Report
@@ Coverage Diff @@
## develop #337 +/- ##
========================================
Coverage 97.95% 97.95%
========================================
Files 25 25
Lines 4447 4447
Branches 746 745 -1
========================================
Hits 4356 4356
Misses 54 54
Partials 37 37
Continue to review full report at Codecov.
|
jklmnn
previously approved these changes
Jul 13, 2020
treiher
requested changes
Jul 13, 2020
senier
pushed a commit
that referenced
this pull request
Jul 13, 2020
treiher
approved these changes
Jul 13, 2020
jklmnn
approved these changes
Jul 13, 2020
treiher
pushed a commit
that referenced
this pull request
Jul 23, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes cases where negative fields length were not detected. The reason was a misconception of what we (need to) proof here. The attempted proof we had in the code so far was something like (
H1
andH2
being the constraints from the range type):We then checked whether a solution could be found:
What this does is checking whether one solution exists for the goal, which is clearly the case for
Length >= 50
. What we actually want and what is implemented now is proving thatLength - 50 < 0
is unsatisfiable.Why wouldn't our tests detect this issue? The error message for a different check (that the start of the field is non-negative) was apparently copied from the length check, but not changed. In addition to the length issue, the test case also had a negative start which triggered that second erroneous error message. As it looked as expected, the ineffectiveness of the length check remained unnoticed. The non-negative test was apparently also copied and checked for the wrong message. Hence, the invalid message remained unnoticed.
Lessons learned:
Other changes:
Since upgrading to
isort 4.3.21
viapip3
on Debian, third-party packages are not detected anymore. As a result,isort
erroneously removes a lot of whitespace. I added our third-party libs to theisort
config, which solves the issue for now and also is more robust if a dependency is not installed.Close #336