Skip to content

Conversation

@MichaReiser
Copy link
Member

Summary

This PR adds initial support for type: ignore. It doesn't do anything fancy yet like:

  • Detecting invalid type ignore comments
  • Detecting type ignore comments that are part of another suppression comment: # fmt: skip # type: ignore
  • Suppressing specific lints type: ignore [code]
  • Detecting unsused type ignore comments
  • ...

The goal is to add this functionality in separate PRs.

Test Plan

@MichaReiser MichaReiser added the ty Multi-file analysis & type inference label Dec 18, 2024
@MichaReiser MichaReiser changed the title Initial type: ignore support Basic support for type: ignore comments Dec 18, 2024
@github-actions
Copy link
Contributor

github-actions bot commented Dec 18, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@MichaReiser MichaReiser marked this pull request as ready for review December 18, 2024 16:16
use crate::{lint::LintId, Db};

#[salsa::tracked(return_ref)]
pub(crate) fn suppressions(db: &dyn Db, file: File) -> IndexVec<SuppressionIndex, Suppression> {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops. I accidentally commited this file when I rebased https://github.com/astral-sh/ruff/pull/14956/files#diff-b1807b646317ac1945d748f7db40451ac62c582b1bbc049b174e8d98f13d3f22 Probably because it didn't get stashed with git stash. So consider all code in this file as new

Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice!

MichaReiser and others added 2 commits December 19, 2024 10:08
Co-authored-by: Carl Meyer <carl@astral.sh>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@MichaReiser MichaReiser marked this pull request as draft December 19, 2024 11:06
@MichaReiser
Copy link
Member Author

I adjusted the implementation to accept type: ignore comments on the start and end lines of the diagnostic range. I understand that this matches mypy's behavior #15046 (comment).

However, this in itself won't be sufficient for mypy compatibility. We'd also have to match mypy's diagnostic range which we currently don't for invalid assignments (#15046 (comment)). But that's a problem out of this PR's scope

@MichaReiser MichaReiser marked this pull request as ready for review December 19, 2024 11:37
@MichaReiser MichaReiser requested a review from carljm December 19, 2024 11:37
Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you for digging into the multi-line behavior here!

cast(int, "test" +
2 # type: ignore
)
+ "other" # TODO: expected-error[invalid-operator]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use / 0 here and then the test would function without a TODO?

Or maybe that muddies the comparison with pyright, which doesn't error on division by zero

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't work with / 0 because Red Knot doesn't support cast yet (and without the cast it's Unknown / 0 which we seem to accept)

@MichaReiser
Copy link
Member Author

Something for tomorrow. I want to take another look why we need to test both the start and the end. Ruff only tests the start. Testing both might be a problem for a future linter if we have

if True:
	print("test")
else: 
	pass  # knot: ignore

because the ignore on the pass would now ignore any error that applies to the entire else-clause.

@MichaReiser
Copy link
Member Author

Not supporting end positions does seem unintuitive. For example, the following wouldn't work

y = (
    4 /
    0  # type: ignore
)

We should push users towards using specific error codes to mitigate the issue that I pointed out in #15046 (comment)

@MichaReiser MichaReiser merged commit 913bce3 into main Dec 20, 2024
21 checks passed
@MichaReiser MichaReiser deleted the micha/type-ignore branch December 20, 2024 09:35
@carljm
Copy link
Contributor

carljm commented Dec 20, 2024

We should push users towards using specific error codes

And try to avoid overly-large diagnostic ranges. In particular, I suspect we should try to avoid diagnostic ranges on statements, because I think those are the cases where the end is least intuitive.

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

Labels

ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants