[ty] Emit a diagnostic on incorrect applications of the legacy convention for specifying positional-only parameters#22943
Conversation
| impl<'a, K, V> IntoIterator for &'a VecMap<K, V> | ||
| where | ||
| K: Eq, | ||
| K: std::fmt::Debug, | ||
| V: std::fmt::Debug, | ||
| { | ||
| type Item = (&'a K, &'a V); | ||
| type IntoIter = VecMapIterator<'a, K, V>; | ||
|
|
||
| fn into_iter(self) -> Self::IntoIter { | ||
| self.iter() | ||
| } | ||
| } | ||
|
|
||
| struct VecMapIterator<'a, K, V> { | ||
| inner: std::slice::Iter<'a, (K, V)>, | ||
| } | ||
|
|
||
| impl<'a, K, V> Iterator for VecMapIterator<'a, K, V> { | ||
| type Item = (&'a K, &'a V); | ||
|
|
||
| fn next(&mut self) -> Option<Self::Item> { | ||
| self.inner.next().map(|(k, v)| (k, v)) | ||
| } | ||
| } | ||
|
|
||
| impl<K, V> std::iter::FusedIterator for VecMapIterator<'_, K, V> {} | ||
|
|
||
| impl<K, V> ExactSizeIterator for VecMapIterator<'_, K, V> { | ||
| fn len(&self) -> usize { | ||
| self.inner.len() | ||
| } | ||
| } |
There was a problem hiding this comment.
These changes are because I was annoyed that I had to explicitly call self.declarations.iter() in order to iterate over self.declarations, rather than just directly iterating over &self.declarations 😄
Typing conformance resultsThe percentage of diagnostics emitted that were expected errors decreased from 81.39% to 81.34%. The percentage of expected errors that received a diagnostic increased from 71.60% to 71.79%. Summary
True positives addedDetails
False positives addedDetails
|
AlexWaygood
force-pushed
the
alex/pre-pep570-lint
branch
from
668edf8 to
07085ca
Compare
|
AlexWaygood
force-pushed
the
alex/pre-pep570-lint
branch
from
07085ca to
334ca87
Compare
|
AlexWaygood
force-pushed
the
alex/pre-pep570-lint
branch
2 times, most recently
from
6cda680 to
d1f51de
Compare
| f"--python-version={python_version}", | ||
| "--output-format=gitlab", | ||
| "--ignore=assert-type-unspellable-subtype", | ||
| "--error=invalid-legacy-positional-parameter", |
There was a problem hiding this comment.
this is necessary because I made the default severity of the lint Level::Warn rather than Level::Error (since it won't fail at runtime)
There was a problem hiding this comment.
I feel like we should set up our conformance suite integration such that warnings count as errors for conformance suite purposes in general.
There was a problem hiding this comment.
That's best done as a separate change, I think
There was a problem hiding this comment.
For example, the number of false positives we report might go up due to some unused-ignore-comment diagnostics on comments in the spec that aren't meant to be suppression comments at all, such as https://github.com/python/typing/blob/08f929ba70397b5c898a55bbb73c8b4e2e8e4fbb/conformance/tests/directives_type_ignore_file1.py#L11. (Related: astral-sh/ty#881.)
I disagree that this is a false positive, and have queried this in the Python Type Checker Builders Discord. The example looks like this: def f3(x: int, *args: int, __y: int) -> None: ... # OKand the comment a few lines above says:
But >>> def f3(x: int, *args: int, __y: int) -> None: ...
...
>>> f3(x=42, __y=56)
>>> |
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-argument-type |
0 | 61 | 1 |
invalid-legacy-positional-parameter |
24 | 0 | 0 |
possibly-missing-attribute |
0 | 6 | 2 |
invalid-parameter-default |
0 | 0 | 7 |
invalid-return-type |
0 | 5 | 1 |
invalid-assignment |
0 | 0 | 1 |
invalid-await |
0 | 1 | 0 |
| Total | 24 | 73 | 12 |
|
Most of the ecosystem hits appear to be true positives. I almost wonder whether this lint should be disabled by default. It's mandated by the spec, but it seems pretty pedantic to me. |
AlexWaygood
force-pushed
the
alex/pre-pep570-lint
branch
from
180c393 to
7310c51
Compare
AlexWaygood
requested review from
MichaReiser,
carljm,
dcreager and
sharkdp
as code owners
| } | ||
|
|
||
| if literal.is_staticmethod(db) { | ||
| if literal.is_staticmethod(db) && literal.name(db) != "__new__" { |
There was a problem hiding this comment.
Is it possible for a method to be a static method, named __new__, but not be the "special" __new__? I.e., is this check too broad? (I assume not?)
There was a problem hiding this comment.
I mean... it's Python, so who knows whether it's possible 😆 But no, I don't think that's something that's realistic for us to worry about or model (and I can't think of a way you could make it happen)
charliermarsh
left a comment
charliermarsh
left a comment
There was a problem hiding this comment.
TIL, never seen this before. I agree with you that this could plausibly be disabled by default.
…tion for specifying positional-only parameters
AlexWaygood
force-pushed
the
alex/pre-pep570-lint
branch
2 times, most recently
from
5ef34c9 to
b20e8c6
Compare
AlexWaygood
force-pushed
the
alex/pre-pep570-lint
branch
from
b20e8c6 to
419e9c9
Compare
| let Some(Type::FunctionLiteral(function_type)) = | ||
| infer_definition_types(db, *definition).undecorated_type() | ||
| else { | ||
| continue; | ||
| }; |
There was a problem hiding this comment.
It feels a bit silly to be calling infer_definition_types for a definition inside this scope, but I think that's the only way to get back the undecorated type of a function that was defined in this scope.
We could also add a raw_signatures: FxHashMap<&'ast ast::StmtFunctionDef, Signature<'db>> field to the TypeInferenceBuilder that could be used to query this information. But I think the infer_definition_types call here should be fairly cheap -- since the definition has already been inferred at this point, it should always be present in the Salsa cache.
Cc. @carljm in case you have opinions!
There was a problem hiding this comment.
This is totally fine, calling a cached Salsa function is very cheap.
| f"--python-version={python_version}", | ||
| "--output-format=gitlab", | ||
| "--ignore=assert-type-unspellable-subtype", | ||
| "--error=invalid-legacy-positional-parameter", |
There was a problem hiding this comment.
I feel like we should set up our conformance suite integration such that warnings count as errors for conformance suite purposes in general.
AlexWaygood
force-pushed
the
alex/pre-pep570-lint
branch
from
b424ac4 to
9c0cd24
Compare
3 participants
Summary
The typing spec states:
This PR adds that lint.
Test Plan
mdtests and snapshots