Always use identifier ranges to store bindings #5110
Merged
Conversation
This file contains 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
PR Check ResultsEcosystemℹ️ ecosystem check detected changes. (+12, -12, 0 error(s)) airflow (+11, -11)
- airflow/callbacks/pipe_callback_sink.py:20:40: TCH003 [*] Move standard library import `multiprocessing.connection.Connection` into a type-checking block
+ airflow/callbacks/pipe_callback_sink.py:20:54: TCH003 [*] Move standard library import `multiprocessing.connection.Connection` into a type-checking block
- airflow/dag_processing/manager.py:36:40: TCH003 [*] Move standard library import `multiprocessing.connection.Connection` into a type-checking block
+ airflow/dag_processing/manager.py:36:54: TCH003 [*] Move standard library import `multiprocessing.connection.Connection` into a type-checking block
- airflow/dag_processing/processor.py:29:40: TCH003 [*] Move standard library import `multiprocessing.connection.Connection` into a type-checking block
+ airflow/dag_processing/processor.py:29:54: TCH003 [*] Move standard library import `multiprocessing.connection.Connection` into a type-checking block
- airflow/executors/kubernetes_executor.py:36:46: TCH002 [*] Move third-party import `kubernetes.client.models` into a type-checking block
+ airflow/executors/kubernetes_executor.py:36:56: TCH002 [*] Move third-party import `kubernetes.client.models` into a type-checking block
- airflow/kubernetes/k8s_model.py:23:31: TCH002 [*] Move third-party import `kubernetes.client.models` into a type-checking block
+ airflow/kubernetes/k8s_model.py:23:41: TCH002 [*] Move third-party import `kubernetes.client.models` into a type-checking block
- airflow/providers/elasticsearch/hooks/elasticsearch.py:29:39: TCH001 [*] Move application import `airflow.models.connection.Connection` into a type-checking block
+ airflow/providers/elasticsearch/hooks/elasticsearch.py:29:53: TCH001 [*] Move application import `airflow.models.connection.Connection` into a type-checking block
+ airflow/providers/exasol/hooks/exasol.py:23:18: TCH002 [*] Move third-party import `pandas` into a type-checking block
- airflow/providers/exasol/hooks/exasol.py:23:8: TCH002 [*] Move third-party import `pandas` into a type-checking block
- airflow/providers/google/cloud/transfers/presto_to_gcs.py:23:28: TCH002 [*] Move third-party import `prestodb.dbapi.Cursor` into a type-checking block
+ airflow/providers/google/cloud/transfers/presto_to_gcs.py:23:38: TCH002 [*] Move third-party import `prestodb.dbapi.Cursor` into a type-checking block
- airflow/providers/google/cloud/transfers/trino_to_gcs.py:23:25: TCH002 [*] Move third-party import `trino.dbapi.Cursor` into a type-checking block
+ airflow/providers/google/cloud/transfers/trino_to_gcs.py:23:35: TCH002 [*] Move third-party import `trino.dbapi.Cursor` into a type-checking block
+ airflow/providers/influxdb/hooks/influxdb.py:27:18: TCH002 [*] Move third-party import `pandas` into a type-checking block
- airflow/providers/influxdb/hooks/influxdb.py:27:8: TCH002 [*] Move third-party import `pandas` into a type-checking block
- airflow/serialization/serialized_objects.py:74:39: TCH004 [*] Move import `kubernetes.client.models` out of type-checking block. Import is used for more than type hinting.
+ airflow/serialization/serialized_objects.py:74:49: TCH004 [*] Move import `kubernetes.client.models` out of type-checking block. Import is used for more than type hinting.bokeh (+1, -1)
+ src/bokeh/plotting/_figure.py:25:17: TCH002 [*] Move third-party import `numpy` into a type-checking block
- src/bokeh/plotting/_figure.py:25:8: TCH002 [*] Move third-party import `numpy` into a type-checking block
BenchmarkLinuxWindows |
charliermarsh
force-pushed
the
charlie/binding-identifier
branch
2 times, most recently
from
9c36a04 to
970107a
Compare
charliermarsh
commented
Jun 15, 2023
| //! The statement defining `f` has a range, but the identifier `f` does not. | ||
| //! | ||
| //! This module assists with extracting [`TextRange`] ranges from AST nodes | ||
| //! via manual lexical analysis. |
There was a problem hiding this comment.
This file got collapsed, but it's really the one that needs to be reviewed. Most of the rest is just API migration.
charliermarsh
force-pushed
the
charlie/binding-identifier
branch
from
970107a to
3a7b3f0
Compare
konstin
approved these changes
Jun 15, 2023
| rest: Some(_), | ||
| .. | ||
| }) => { | ||
| if let Some(pattern) = patterns.last() { |
There was a problem hiding this comment.
Suggested change
| if let Some(pattern) = patterns.last() { | |
| // From https://docs.python.org/3/reference/compound_stmts.html#grammar-token-python-grammar-mapping_pattern | |
| // > At most one double star pattern may be in a mapping pattern. | |
| // > The double star pattern must be the last subpattern in the mapping pattern. | |
| if let Some(pattern) = patterns.last() { |
You might want to change the exact comment, but i expect many readers won't be familiar with what exactly the PatternMatchMapping node entails
|
|
||
| /// Peeks the next character from the input stream without consuming it. | ||
| /// Returns [`EOF_CHAR`] if the file is at the end of the file. | ||
| fn first(&self) -> char { |
There was a problem hiding this comment.
Why does this return EOF while bump returns an option
There was a problem hiding this comment.
This is consistent with how Micha designed it in the trivia.rs (from which this is derived), so I'm just gonna leave it for now.
charliermarsh
force-pushed
the
charlie/binding-identifier
branch
from
3a7b3f0 to
7945a9f
Compare
charliermarsh
force-pushed
the
charlie/binding-identifier
branch
from
7945a9f to
43a26ca
Compare
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.
Summary
At present, when we store a binding, we include a
TextRangealongside it. TheTextRangesometimes matches the exact range of the identifier to which theBindingis linked, but... not always.For example, given:
The binding we create will use the range of
x, because the left-hand side is anExpr::Name, which has a valid range on it.However, given:
When we create a binding for
e, we don't have aTextRange... The AST doesn't give us one. So we end up extracting it via lexing.This PR extends that pattern to the rest of the binding kinds, to ensure that whenever we create a binding, we always use the range of the bound name. This leads to better diagnostics in cases like pattern matching, whereby the diagnostic for "unused variable
x" here used to include*x, instead of justx:This is also required for symbol renames, since we track writes as bindings -- so we need to know the ranges of the bound symbols.
By storing these bindings precisely, we can also remove the
binding.trimmed_rangeabstraction -- since bindings already use the "trimmed range".To implement this behavior, I took some of our existing utilities (like the code we had for
except ValueError as eabove), migrated them from a full lexer to a zero-allocation lexer that only identifies "identifiers", and moved the behavior into a trait, so we can now dostmt.identifier(locator)to get the range for the identifier.Honestly, we might end up discarding much of this if we decide to put ranges on all identifiers (astral-sh/RustPython-Parser#8). But even if we do, this will still be a good change, because the lexer introduced here is useful beyond names (e.g., we use it find the
exceptkeyword in an exception handler, to find theelseafter aforloop, and so on). So, I'm fine committing this even if we end up changing our minds about the right approach.Closes #5090.
Benchmarks
No significant change, with one statistically significant improvement (-2.1654% on
linter/all-rules/large/dataset.py):