Add unguarded-next-without-default
#6987
Closed
Conversation
This file contains hidden or 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
Pull Request Test Coverage Report for Build 2534764659
💛 - Coveralls |
This comment has been minimized.
This comment has been minimized.
|
I checked some the primer changes and they seem fine. One issue is the second example in |
|
This one (using |
Pierre-Sassoulas
requested changes
Jun 21, 2022
There was a problem hiding this comment.
Related PEP that I think we should talk about : https://peps.python.org/pep-0479/
I think we need to check the scope of the routine where the (except if it's a function that is supposed to raise next is used to verify that the StopIteration is caughtStopIteration like __next__). We also need to check that py-version is > 3.7. Raìsing a RuntimeError implicitly is bad, doing anything else explicitly after catching the StopIterationis okay.
I don't think Edit: Never mind. You meant that raising |
👍
Shouldn't that be a different check? As this is a Warning and |
Co-authored-by: Pierre Sassoulas <[email protected]>
DanielNoord
commented
Jun 21, 2022
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Pierre Sassoulas <[email protected]>
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
There's still a lot of false positive in most of the primer repository. I'm wondering if we should:
- infer the value of the generator so we do not raise if it's obvious it will always be possible to call next on it (Using next directly is reasonable to if the generator is defined in the same scope ?)
- Put this in an extension, currently it's a default check and seeing the number of warning in working libraries it's a little too naive or opinionated right now
I'm going to think a bit about this and see if I can come up with something. On the other hand, isn't this also what a linter is for: catching code that might work now, but is not always safe in the future? Stuff like |
|
For some thing we also assume the user knows what he does, i.e. we rejected a check for dict access without using |
Yeah, I'm just wondering what options users have to signal that they know the risk of
Using a variable that's defined in the same scope seems like it should also be one, but I don't think |
I'd say this one could be "not activating the extensions that check this" or "disabling the default check". I think if we want to minimize false positives against false negatives the former is better.
Can we infer the generator length and compare against the number of call to |
|
@jacobtylerwalls Do you have an opinion about this? I'm still not sure whether I think this should be turned on by default. Take for example the following hit on This seems fine: there is an |
jacobtylerwalls
changed the title
unspecified-default-for-nextunguarded-next-without-default
|
Let's block this on an astroid Constraint PR that adds a not-empty constraint. That would deal with most of the false positives, right? Then, if there are still code paths that make an empty generator reachable by an unguarded next, that's fine to warn about, even if they're unlikely paths in working libraries. People will just disable the whole check if it's too picky, but I think they'll see the value for others and won't complain :D |
This comment has been minimized.
This comment has been minimized.
Pierre-Sassoulas
removed
Blocked 🚧
This comment has been minimized.
This comment has been minimized.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6987 +/- ##
=======================================
Coverage 95.43% 95.43%
=======================================
Files 176 176
Lines 18545 18557 +12
=======================================
+ Hits 17698 17710 +12
Misses 847 847
|
This comment has been minimized.
This comment has been minimized.
|
🤖 Effect of this PR on checked open source code: 🤖 Effect on astroid:
Effect on black:
Effect on django:
Effect on flask:
Effect on music21:
Effect on pandas:
Effect on pytest:
Effect on sentry:
Effect on coverage:
This comment was generated for commit da4074e |
Pierre-Sassoulas
requested changes
Feb 5, 2023
| @@ -0,0 +1 @@ | |||
| next(i for i in (1, 2) if isinstance(i, str)) # [unguarded-next-without-default] | |||
There was a problem hiding this comment.
Suggested change
| next(i for i in (1, 2) if isinstance(i, str)) # [unguarded-next-without-default] | |
| def display(animals): | |
| iterator = iter(animals) | |
| while True: | |
| print(next(iterator)) # [unguarded-next-without-default] |
| @@ -0,0 +1 @@ | |||
| next((i for i in (1, 2) if isinstance(i, str)), None) | |||
There was a problem hiding this comment.
Suggested change
| next((i for i in (1, 2) if isinstance(i, str)), None) | |
| def display(animals): | |
| iterator = iter(animals) | |
| while True: | |
| next_animal = next(iterator, None) | |
| if next_animal is None: | |
| break | |
| print(next_animal) |
| "unguarded-next-without-default", | ||
| "Without a default value calls to next() can raise a StopIteration " | ||
| "exception. This exception should be caught or a default value should " | ||
| "be provided.", |
There was a problem hiding this comment.
Suggested change
| "be provided.", | |
| "be provided, unless you're in a function that is expected to raise ``StopIteration``.", |
| }, | ||
| ), | ||
| "W1519": ( | ||
| "Using next without explicitly specifying a default value or catching the StopIteration", |
There was a problem hiding this comment.
Suggested change
| "Using next without explicitly specifying a default value or catching the StopIteration", | |
| "Using next without specifying a default value or catching the StopIteration", |
There's no implicit default, it's going to raise a StopIteration if you don't set the default right ?
| "W1519": ( | ||
| "Using next without explicitly specifying a default value or catching the StopIteration", | ||
| "unguarded-next-without-default", | ||
| "Without a default value calls to next() can raise a StopIteration " |
There was a problem hiding this comment.
Suggested change
| "Without a default value calls to next() can raise a StopIteration " | |
| "Without a default value calls to next() will raise a ``StopIteration`` " |
| "Using next without explicitly specifying a default value or catching the StopIteration", | ||
| "unguarded-next-without-default", | ||
| "Without a default value calls to next() can raise a StopIteration " | ||
| "exception. This exception should be caught or a default value should " |
There was a problem hiding this comment.
Suggested change
| "exception. This exception should be caught or a default value should " | |
| "exception when there's nothing to iterate anymore. It's generally not a trivial task to prove that it's possible to call next safely so this exception should be caught or a default value should " |
Comment on lines
+35
to
+37
| class MyClass: | ||
| def __next__(self): | ||
| return next(i for i in (1, 2)) |
There was a problem hiding this comment.
Suggested change
| class MyClass: | |
| def __next__(self): | |
| return next(i for i in (1, 2)) | |
| class MyClass: | |
| def __next__(self): | |
| return next(i for i in (1, 2)) | |
| def expected_to_raise_stop_iteration(self): | |
| """Custom iterator function that will raise StopIteration by design. | |
| Raises: | |
| StopIteration: If the end of the sequence has been reached. | |
| """ | |
| return next(i for i in (1, 2)) |
Just a thought. Maybe we don't need to check that it's a proper docstring in numpy style or whatever. If there's StopIteration in the docstring we do not raise.
|
I thought we identified a class of false positives we could address with a not-empty constraint in astroid? |
|
Astroid inferring capability with none check has been improved in 2.13. there's still a lot of warning in the primers, I did not check how many less compared to before. Do we want to check what is inside the iterator to raise less and more accurately ? (I.e. is this a errors message or a convention/warning message ?) |
|
Sorry for not fleshing out further. I was suggesting blocking this PR on a yet to be written astroid PR that would build on the last constraint PR ( bunch = []
if bunch:
next(bunch) # just doing this once, and we just checked not-emptyAlso, I think this PR should also special case the infinite iterator used here: https://github.com/nedbat/coveragepy/blob/b059a67fd1fe5d514f7c283f5ab99052e1cea15f/coverage/debug.py#L414 |
|
Maybe it can be done inside pylint only, without another astroid PR, I don't know. |
|
This would require some work in |
|
Going to close this. I never had time to look into this further and the amount of false positives without changes to |
|
It's a pity, that would be a nice check :( But yeah, if you use next without default you probably thought about what you're doing thus a lot of false positives. |
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.
doc/whatsnew/2/2.15/index.rst(ordoc/whatsnew/2/2.14/full.rstif the change needs backporting in 2.14). If necessary you can write
details or offer examples on how the new change is supposed to work.
and preferred name in
script/.contributors_aliases.jsonType of Changes
Description
Closes #4725.