gh-156047: Rank tokens before probing for a keyword typo - #156087
gh-156047: Rank tokens before probing for a keyword typo#156087johnslavik wants to merge 2 commits into
Conversation
|
FYI here's logs of what tokens are examined in the typo cases from our tests: DetailsWe can tweak the rank calculation a bit to look for a token on the left first but I don't think it's a huge deal in this context. |
pablogsal
left a comment
There was a problem hiding this comment.
Hummmm…. I am not very convinced about this method. It’s true that’s a bit better but it is still not without inconsistencies
| the_end = end_line if line == 0 else end_line + 1 | ||
| if token.start[0] + line != the_end: | ||
| continue | ||
| rank = abs(offset - token.end[1]) |
There was a problem hiding this comment.
I think the rank anchor is a bit off here: self.end_offset is 1-based and relative to the original line, but token.end[1] is 0-based and relative to the dedented snippet, so for indented code the anchor drifts right by the dedent width and the neighbours get probed before the typo (you can see it in your traces, where x ranks above iff). Not incorrect because every candidate is validated by recompiling, but can we translate end_offset to the snippet coordinates before ranking?
There was a problem hiding this comment.
I'll also add a case where dedent() makes a difference. For the TYPO_CASES we had, dedent() was a no-op so this was harder to spot.
| the_end = end_line if line == 0 else end_line + 1 | ||
| if token.start[0] + line != the_end: | ||
| continue | ||
| rank = abs(offset - token.end[1]) |
There was a problem hiding this comment.
This can prefer the token at the parser caret over the actual typo. retrun a + b now suggests and, because replacing a makes the expression compile. We need a ranking that keeps retrun first and a test for this case.
There was a problem hiding this comment.
Somewhat related fun fact, you can trigger this in main too:
def outer():
if True:
pass
retrun a+bThe dedented block includes only the body of the function without the def, so retrun->return won't compile since it is "outside function".
There was a problem hiding this comment.
I know I'm getting off-topic but in fact, any functions which have a body with first block being correct syntax and return / yield / await anywhere in that body, won't have any keyword typo suggestions at all, and it's a pre-existing bug. Probably not worth fixing now, since it would either require creating a virtual async function block and another layer of offset translation or adding new flags to Nevermind, we can just strip compile() to allow all function-only keywords be top-level (the second is a much cleaner fix)...lines more intelligently when we create error_code!
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
m-aciek
left a comment
There was a problem hiding this comment.
I found one regression, and left suggestion for the exception catch. It's nice it covers more cases, I hope it is possible to make it work with multiline still?
| except Exception: | ||
| pass |
There was a problem hiding this comment.
| except Exception: | |
| pass | |
| except tokenize.TokenError: | |
| # Incomplete input can still contain useful tokens. | |
| pass |
(non-blocking) TokenError handling is safer than catch-all because it continues only for the incomplete-input condition where partial tokens are known to be useful.
There's a catch-all in the caller already
Line 1561 in 5a0ab76
There was a problem hiding this comment.
In short: because we unroll the tokens early and process them later in potentially different order, it's best to catch-all on the unrolling part and then always move on to processing.
Details
The idea here is to avoid bailing out if an iteration of the loop fails for any reason, and still process any tokens gathered up until that failure.
While TokenError might be the only possible exception reachable here right now, the invariant is broader than that. The intent is to avoid bailing in case of any exception raised while iterating in this loop, since there can still be tokens to test prior to failure.
To illustrate this: if we kept the status quo, whatever exception during iterating wouldn't stop tokens from being processed, and we want to preserve this property here regardless of the current implementation details. While we may argue that this code will never raise anything else than TokenError ever, broader guard reflects the intent clearer, and is immune to drifts in the implementation details (i.e. from some point in the future something other than TokenError might be raised).
There's a catch-all in the caller already
Line 1561 in 5a0ab76
In this PR, the catch-all in the caller becomes irrelevant to avoiding bailing out specifically before the tokens are processed. The purpose is to ignore whatever exception and still process any tokens that we managed to collect prior to failure.
| # Only consider NAME tokens on the same line as the error | ||
| the_end = end_line if line == 0 else end_line + 1 | ||
| if token.start[0] + line != the_end: | ||
| continue |
There was a problem hiding this comment.
Restricting ourselves to only tokens on the same line makes us ignore valid suggestions:
iff \
x:
pass
The parser reports line 2, but the typo is on line 1.
- main:
Did you mean 'if'? - PR: no suggestion
Candidates on the error line should be prioritized, not made the exclusive candidate set.
There was a problem hiding this comment.
And we should cover this case as well.
| if hint: | ||
| matches.append(hint) | ||
| if _suggestions is not None: | ||
| suggestion = _suggestions._generate_suggestions(keyword.kwlist + keyword.softkwlist, wrong_name) | ||
| if suggestion: | ||
| matches.append(suggestion) | ||
| matches.extend( | ||
| difflib.get_close_matches( | ||
| wrong_name, | ||
| keyword.kwlist + keyword.softkwlist, | ||
| n=max_matches, | ||
| cutoff=0.5 | ||
| ) | ||
| ) |
There was a problem hiding this comment.
This had a pre-existing bug of accumulating duplicate replacement candidates.
E.g. for retrun, return will be checked twice (matches will contain 2 'return' strings)!
I'd like to fix it in this PR too.
@pablogsal Look at this! Amazing!
cc @m-aciek