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.

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

Track ranges of names inside __all__ definitions #10525

Merged
merged 4 commits into from
Mar 22, 2024
Merged

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Mar 22, 2024

Summary

This resolves the TODO comment here:

// Mark anything referenced in `__all__` as used.
// TODO(charlie): `range` here should be the range of the name in `__all__`, not
// the range of `__all__` itself.

Fixing this TODO is necessary (though not quite sufficient) for fixing #10508.

This PR adds a new struct for representing __all__ members to crates/ruff_python_ast/all.rs:

struct DunderAllMember<'a> {
    name: &'a str,
    range: TextRange
}

I initially just kept references to ExprStringLiteral nodes around, since they have all the information we need here, but apparently that approach leads to a large performance regression 🙃

Test Plan

cargo test

Copy link

codspeed-hq bot commented Mar 22, 2024

CodSpeed Performance Report

Merging #10525 will not alter performance

Comparing dunder-all-ranges (a6fc510) with main (4f06d59)

Summary

✅ 30 untouched benchmarks

Copy link
Contributor

github-actions bot commented Mar 22, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@AlexWaygood AlexWaygood marked this pull request as draft March 22, 2024 15:33
@AlexWaygood AlexWaygood marked this pull request as ready for review March 22, 2024 15:50
@AlexWaygood
Copy link
Member Author

I fixed the performance regressions initially reported by codspeed, and edited the PR description to reflect the new approach.

@MichaReiser MichaReiser added bug Something isn't working fixes Related to suggested fixes for violations labels Mar 22, 2024
@AlexWaygood AlexWaygood merged commit a06ffeb into main Mar 22, 2024
17 checks passed
@AlexWaygood AlexWaygood deleted the dunder-all-ranges branch March 22, 2024 18:38
charliermarsh added a commit that referenced this pull request Apr 6, 2024
## Summary

Historically, given:

```python
__all__ = [  # noqa: F822
    "Bernoulli",
    "Beta",
    "Binomial",
]
```

The F822 violations would be attached to the `__all__`, so this `# noqa`
would be enforced for _all_ definitions in the list. This changed in
#10525 for the better, in that we
now use the range of each string. But these `# noqa` directives stopped
working.

This PR sets the `__all__` as a parent range in the diagnostic, so that
these directives are respected once again.

Closes #10795.

## Test Plan

`cargo test`
Glyphack pushed a commit to Glyphack/ruff that referenced this pull request Apr 12, 2024
## Summary

Historically, given:

```python
__all__ = [  # noqa: F822
    "Bernoulli",
    "Beta",
    "Binomial",
]
```

The F822 violations would be attached to the `__all__`, so this `# noqa`
would be enforced for _all_ definitions in the list. This changed in
astral-sh#10525 for the better, in that we
now use the range of each string. But these `# noqa` directives stopped
working.

This PR sets the `__all__` as a parent range in the diagnostic, so that
these directives are respected once again.

Closes astral-sh#10795.

## Test Plan

`cargo test`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixes Related to suggested fixes for violations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants