Skip to content
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.

Sign up for GitHub

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

Jump to bottom

UP037: autofix removes quotes from Literal["value"] typing when not imported #7025

Open
Prometheos2 opened this issue Aug 31, 2023 · 7 comments
Open

UP037: autofix removes quotes from Literal["value"] typing when not imported #7025

Prometheos2 opened this issue Aug 31, 2023 · 7 comments
Labels
needs-decision Awaiting a decision from a maintainer

Comments

@Prometheos2
Copy link

UP037's autofix removes quotes from typing.Literal["value"] when Literal is not imported, but it's supposed to have str values.

_DTYPE_BACKEND: Literal["pyarrow", "numpy_nullable"]

turns into

_DTYPE_BACKEND: Literal[pyarrow, numpy_nullable]

Details

  • Version: 0.0.286
  • Relevant settings: select = ["UP"]
  • Commands producing the bug:
    • the VSC extension's autofix
    • ruff . --fix
    • ruff . --fix --isolated --select UP
@charliermarsh
Copy link
Member

Is it a mistake or intentional that Literal is not imported?

@charliermarsh charliermarsh added the needs-info More information is needed from the issue author label Aug 31, 2023
@Prometheos2
Copy link
Author

It was a mistake.

(Admittedly, it was a bit of both. I was trying out Ruff's autofix on save on VSC.)

@zanieb
Copy link
Member

zanieb commented Aug 31, 2023 •
edited
Loading

The problem here is that we cannot identify Literal as typing.Literal if it's not imported. We could add an exception for this to avoid false positives but it could introduce false negatives. Perhaps we can just avoid applying this fix (or reduce the applicability) if there's not a binding for the name of the subscript?

@charliermarsh
Copy link
Member

Yeah this is largely "working as intended" in that if we can't resolve a symbol to typing.Literal, then we treat values within subscripts in type annotations as type annotations themselves (typing.Literal is an exception).

I suppose we could change the logic to avoid treating values within subscripts as annotations if the subscript itself is unbound, although it would do the wrong thing if the user has any wildcard imports.

@charliermarsh charliermarsh added needs-decision Awaiting a decision from a maintainer and removed needs-info More information is needed from the issue author labels Aug 31, 2023
@zanieb
Copy link
Member

zanieb commented Sep 1, 2023

Perhaps we can downgrade from automatic to suggested fix if it's unbound. The wildcard imports are a good example, but hopefully not too common in this case?

@zanieb zanieb mentioned this issue Oct 3, 2023
Merged
zanieb added a commit that referenced this issue Oct 6, 2023
@zanieb @evanrittenhouse
Update CLI to respect fix applicability (#7769)
22e1874 
Rebase of #5119 authored by
@evanrittenhouse with additional refinements.

## Changes

- Adds `--unsafe-fixes` / `--no-unsafe-fixes` flags to `ruff check`
- Violations with unsafe fixes are not shown as fixable unless opted-in
- Fix applicability is respected now
    - `Applicability::Never` fixes are no longer applied
    - `Applicability::Sometimes` fixes require opt-in
    - `Applicability::Always` fixes are unchanged
- Hints for availability of `--unsafe-fixes` added to `ruff check`
output

## Examples

Check hints at hidden unsafe fixes
```
❯ ruff check example.py --no-cache --select F601,W292
example.py:1:14: F601 Dictionary key literal `'a'` repeated
example.py:2:15: W292 [*] No newline at end of file
Found 2 errors.
[*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option).
```

We could add an indicator for which violations have hidden fixes in the
future.

Check treats unsafe fixes as applicable with opt-in
```
❯ ruff check example.py --no-cache --select F601,W292 --unsafe-fixes
example.py:1:14: F601 [*] Dictionary key literal `'a'` repeated
example.py:2:15: W292 [*] No newline at end of file
Found 2 errors.
[*] 2 fixable with the --fix option.
```

Also can be enabled in the config file

```
❯ cat ruff.toml
unsafe-fixes = true
```

And opted-out per invocation

```
❯ ruff check example.py --no-cache --select F601,W292 --no-unsafe-fixes
example.py:1:14: F601 Dictionary key literal `'a'` repeated
example.py:2:15: W292 [*] No newline at end of file
Found 2 errors.
[*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option).
```

Diff does not include unsafe fixes
```
❯ ruff check example.py --no-cache --select F601,W292 --diff
--- example.py
+++ example.py
@@ -1,2 +1,2 @@
 x = {'a': 1, 'a': 1}
-print(('foo'))
+print(('foo'))
\ No newline at end of file

Would fix 1 error.
```

Unless there is opt-in
```
❯ ruff check example.py --no-cache --select F601,W292 --diff --unsafe-fixes
--- example.py
+++ example.py
@@ -1,2 +1,2 @@
-x = {'a': 1}
-print(('foo'))
+x = {'a': 1, 'a': 1}
+print(('foo'))
\ No newline at end of file

Would fix 2 errors.
```

#7790 will improve the diff
messages following this pull request

Similarly, `--fix` and `--fix-only` require the `--unsafe-fixes` flag to
apply unsafe fixes.

## Related

Replaces #5119
Closes #4185
Closes #7214
Closes #4845
Closes #3863
Addresses #6835
Addresses #7019
Needs follow-up #6962
Needs follow-up #4845
Needs follow-up #7436
Needs follow-up #7025
Needs follow-up #6434
Follow-up #7790 
Follow-up #7792

---------

Co-authored-by: Evan Rittenhouse <[email protected]>
@zanieb
Copy link
Member

zanieb commented Oct 6, 2023

I'm not sure how to implement this with the way that we visit type definitions — do you have a suggestion @charliermarsh ?

konstin pushed a commit that referenced this issue Oct 11, 2023
@zanieb @evanrittenhouse @konstin
Update CLI to respect fix applicability (#7769)
c4cd510 
Rebase of #5119 authored by
@evanrittenhouse with additional refinements.

## Changes

- Adds `--unsafe-fixes` / `--no-unsafe-fixes` flags to `ruff check`
- Violations with unsafe fixes are not shown as fixable unless opted-in
- Fix applicability is respected now
    - `Applicability::Never` fixes are no longer applied
    - `Applicability::Sometimes` fixes require opt-in
    - `Applicability::Always` fixes are unchanged
- Hints for availability of `--unsafe-fixes` added to `ruff check`
output

## Examples

Check hints at hidden unsafe fixes
```
❯ ruff check example.py --no-cache --select F601,W292
example.py:1:14: F601 Dictionary key literal `'a'` repeated
example.py:2:15: W292 [*] No newline at end of file
Found 2 errors.
[*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option).
```

We could add an indicator for which violations have hidden fixes in the
future.

Check treats unsafe fixes as applicable with opt-in
```
❯ ruff check example.py --no-cache --select F601,W292 --unsafe-fixes
example.py:1:14: F601 [*] Dictionary key literal `'a'` repeated
example.py:2:15: W292 [*] No newline at end of file
Found 2 errors.
[*] 2 fixable with the --fix option.
```

Also can be enabled in the config file

```
❯ cat ruff.toml
unsafe-fixes = true
```

And opted-out per invocation

```
❯ ruff check example.py --no-cache --select F601,W292 --no-unsafe-fixes
example.py:1:14: F601 Dictionary key literal `'a'` repeated
example.py:2:15: W292 [*] No newline at end of file
Found 2 errors.
[*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option).
```

Diff does not include unsafe fixes
```
❯ ruff check example.py --no-cache --select F601,W292 --diff
--- example.py
+++ example.py
@@ -1,2 +1,2 @@
 x = {'a': 1, 'a': 1}
-print(('foo'))
+print(('foo'))
\ No newline at end of file

Would fix 1 error.
```

Unless there is opt-in
```
❯ ruff check example.py --no-cache --select F601,W292 --diff --unsafe-fixes
--- example.py
+++ example.py
@@ -1,2 +1,2 @@
-x = {'a': 1}
-print(('foo'))
+x = {'a': 1, 'a': 1}
+print(('foo'))
\ No newline at end of file

Would fix 2 errors.
```

#7790 will improve the diff
messages following this pull request

Similarly, `--fix` and `--fix-only` require the `--unsafe-fixes` flag to
apply unsafe fixes.

## Related

Replaces #5119
Closes #4185
Closes #7214
Closes #4845
Closes #3863
Addresses #6835
Addresses #7019
Needs follow-up #6962
Needs follow-up #4845
Needs follow-up #7436
Needs follow-up #7025
Needs follow-up #6434
Follow-up #7790 
Follow-up #7792

---------

Co-authored-by: Evan Rittenhouse <[email protected]>
@charliermarsh
Copy link
Member

@zanieb - What if we just treat "Literal" as typing.Literal if the symbol is unresolved...?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
needs-decision Awaiting a decision from a maintainer
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@charliermarsh @zanieb @Prometheos2