Add unique typo corrections to the main diagnostic with a fix-it #15800
Conversation
|
@swift-ci Please test. |
|
Well, we don't recover at all; if that's a hard requirement, we can't do this yet. The "almost certainly correct" threshold is interesting for typo correction, because there are certainly a non-trivial number of places where it's wrong. |
|
So we do have a lot of existing corrections that do not trigger recovery. diagnoseUnviableLookupResults just generally has no way to actually trigger a recovery, but it adds a lot of fix-its on its primary errors. |
lib/Sema/TypeCheckNameLookup.cpp
Outdated
| Candidates.clear(); | ||
|
|
||
| return TypoCorrection(WrittenName, Loc, uniqueCorrectedName.getBaseName()); | ||
| } |
lib/Sema/TypeCheckNameLookup.cpp
Outdated
| // noteAllCorrections(). | ||
| Candidates.clear(); | ||
|
|
||
| return TypoCorrection(WrittenName, Loc, uniqueCorrectedName.getBaseName()); |
There was a problem hiding this comment.
I'm a bit surprised that TypoCorrection doesn't include the candidate itself. The clients truly don't even bother to try to use that result to continue meaningfully?
There was a problem hiding this comment.
Oh, I guess they'll do the lookup again with the corrected name, which is likely safer.
There was a problem hiding this comment.
It's more that TypoCorrection is supposed to be basically "everything you would add to the diagnostic" more than it's meant to be a semantically self-describing thing, with the expectation that this will make it really easy to adopt typo corrections into diagnostics without repeating a lot of stuff. For example, we actually immediately break down the DeclName of the candidate to just the base name on initialization because we currently never typo-correct anything about the argument labels.
You're right that having the candidate there would help with recovery. On the other hand, promising to have a candidate there would prevent us from using this path in cases where there's a unique typo correction but it resolves to an overload set.
| excuse = .tired | ||
| excuse = .tooHard // FIXME: should provide Fix-It expected-error{{type 'HomeworkExcuse' has no member 'tooHard'}} {{none}} | ||
| excuse = .tooHard // expected-error{{type 'HomeworkExcuse' has no member 'tooHard'; did you mean 'TooHard'?}} {{13-20=TooHard}} |
|
|
||
| /// Look for a single candidate in this set that matches the given predicate. | ||
| template <class Fn> | ||
| ValueDecl *getUniqueCandidateMatching(Fn &&predicate) { |
There was a problem hiding this comment.
The API here is a bit inconsistent. claimUniqueCorrection() only produces the name, so the client needs to perform lookup again with the corrected name. getUniqueCandidateMatching() doesn't de-duplicate things with the same name, but does return the declaration... which skips the extra lookup in the client (and the validation that goes with performing that lookup directly). It seems to me like the "matching" variant should work the same as claimUniqueCorrection() (de-duplicate and only return the corrected name), with the added predicate check.
There was a problem hiding this comment.
I've revised this a bit. The candidates are now left in the TypoCorrectionResults, and we just change how we note them. This means that recovery can proceed by just taking the candidates out of the results set.
A TypoCorrection should not carry a candidate because we might have a unique correction that corresponds to multiple candidates. This should also be clearer now, I hope.
|
I guess I just don't find this motivating. If there's only one candidate, it'll still show up with a fix-it in Xcode, and it's not that much more info on the command line. |
rjmccall
force-pushed
the
typo-correction-error-fixit
branch
3 times, most recently
from
f0496ce to
e22e4cc
Compare
|
@swift-ci Please test. |
|
Build failed |
|
Build failed |
rjmccall
force-pushed
the
typo-correction-error-fixit
branch
from
e22e4cc to
5ce1df8
Compare
|
@swift-ci Please test. |
|
Build failed |
|
Build failed |
|
@swift-ci Please test OS X. |
1 similar comment
|
@swift-ci Please test OS X. |
Continue to emit notes for the candidates, but use different text. Note that we can emit a typo correction fix-it even if there are multiple candidates with the same name. Also, disable typo correction in the migrator, since the operation is quite expensive, the notes are never presented to the user, and the fix-its can interfere with the migrator's own edits. Our general guidance is that fix-its should be added on the main diagnostic only when the fix-it is highly likely to be correct. The exact threshold is debateable. Typo correction is certainly capable of making mistakes, but most of its edits are right, and when it's wrong it's usually obviously wrong. On balance, I think this is the right thing to do. For what it's worth, it's also what we do in Clang.
rjmccall
force-pushed
the
typo-correction-error-fixit
branch
from
5ce1df8 to
7815892
Compare
|
@swift-ci Please test. |
1 similar comment
|
@swift-ci Please test. |
|
@swift-ci Please test OS X. |
|
I guess I'm just not going to get a test of OS X today. |
|
@swift-ci Please test OS X. |
|
Build failed |
| .highlight(UDRE->getSourceRange()); | ||
| }; | ||
|
|
||
| bool claimed = false; |
There was a problem hiding this comment.
This is unused; is it meant to be used for something or just vestigial?
Only do this if the correction is simple enough that there's no need to print it separately.
Also, disable typo correction in the migrator, since the notes are never seen and the fix-its interfere with what the migrator's trying to do.