[RFC] Removing support for delayed typo correction (original) (raw)

A few weeks ago, I posted about the need for a maintainer for delayed typo correction due to the number of open issues with it and the complexity of the feature. To date, nobody has stepped up. During those two weeks, at least two new issues were filed about crashes with delayed typo expressions.

This is not the first time people have observed the fragility of this feature, so I am proposing we remove support for this functionality (just delayed typo correction, not all typo correction) due to it being unmaintained and known to be problematic for years at this point. There are two ways we could go: 1) just rip it out, 2) hide it behind an off-by-default flag. I am proposing we rip it out entirely; I don’t expect folks to enable the flag often enough for it to be worth keeping, and the fragility issues would still be present so long as users can opt into the behavior.

This feature impacts about 120 tests in Clang. As I was updating tests, I found the changes were a mixture of positives and negatives. If you would like to see what the impacts are in more detail, I have a branch that puts the support behind a cc1-only flag. That branch corrects an OpenMP parsing issue which is likely separable. It does not attempt to improve the modules diagnostic behavior; that is expected to be left as an open issue. But a summary of what I found is:

• Perhaps obvious, but we no longer emit a lot of did you mean <X>? diagnostics. However, what we chose to show the user was a typo correction based solely on edit distance to another identifier, not whether the replacement actually made sense. So in several (but not all) cases, dropping this text actually improves the diagnostic behavior. Typical example, Example of unhelpful suggestion that no longer happens
• Sometimes follow-on diagnostics are no longer emitted, to positive effect as they were effectively noise. Example of unhelpful follow-on that is no longer emitted
• The biggest negative by far is that one particular kind of modules diagnostic got significantly worse. We use delayed typo correction as the mechanism by which we emit diagnostics along the lines of “you need to import this module for that identifier to be visible” which are now replaced by “unknown identifier” diagnostics, which is an unfortunate regression. Example

However, on balance, I believe the net for removal is positive (neutral at worst). Removing this functionality fixes at least 20 open issues (verified by adding new test cases), all of which were crashing bugs but several of which were also compile time performance related due pathological lookup behavior (but this is only on the “cold” path when we’re issuing a diagnostic) and while this does lose some helpful “did you mean” diagnostics, still leaves the compiler in a very usable state. But if the modules diagnostic behavior is sufficiently concerning, we could leave the functionality controlled by a flag for folks to opt into. But my intuition is that the flag would almost never be enabled in practice and so it’s likely better to just pull the functionality entirely.

Note: this RFC is not suggesting we don’t support typo correction. We’ll continue to support the immediate typo corrections we already have today, and if anyone comes along to improve that behavior, we’ll gladly consider those PRs. This is purely about removing unmaintained functionality that is causing problems in practice to improve the overall stability of the toolchain. This functionality can be brought back later if there’s interest.

The question I would like input from the community on is: given that it’s fragile and we have nobody stepping up to maintain it, should we rip it out entirely or should we hide it behind a flag? (Or, does this cause enough consternation that someone is willing to volunteer as maintainer for delayed typo correction?)

I think the modules concern is the only real ‘shame’ to this. It would be great if our Modules owners could comment on whether these diagnostics are things that were put there for convenience, or out of necessity, and whether this is something we could implement differently in a more reliable manner.

I would very much NOT want this to be a flag. It is already a bug-farm as you’ve seen, and putting it behind a flag will just result in it being a bug-farm that is hidden behind a flag: so it’ll result in much less visibility for even more bugs.

I’m very much in favor of remove entirely unless someone is willing to step up, repair a vast majority of the bugs we’ve found with it, and continue maintaining it.

shafik May 30, 2025, 6:19pm 3

As someone who spends a significant amount of time triaging bug reports. Having a set of bugs that we won’t realistically get addressed and yet keep still coming in is a burden. The large number of outstanding bugs means this is having a real negative affect on our users as well. This does not seem like a good use of our very limited time.

I took a quick look at these modules regressions, and I think they will work fine with normal typo correction. I tried this by modifying CorrectTypoDelayed to return nullptr, but also changed DiagnoseEmptyLookup to take the branch that uses non-delayed CorrectTypo instead. With that change there is only one failure in a test/Modules, and it looks like an improvement (removing a duplicate error). Note: I haven’t explored what non-modules impact that has.

I agree with you that having it behind a flag is just gonna aggravate the bitrot.
We should either fix it or rip it out, with the understanding that once it’s removed, it’s very unlikely to come back.

Looking at the changed tests, there are a few that get worse, and it’s a bit of a shame. I wish we would spend a bit more time investigating the bugs before pulling the plug.
At the same time, I can’t commit time to do so… and if no one is willing to… I guess we don’t have many options.

From our experience with Clangd and Clang at Google over the years, delayed typo correction definitely was high on the list of root causes for crashes we observed. I believe we also saw some performance problems back in the day (before 2020), but I may misremember as it’s definitely not an issue today.

I agree with others that loosing diagnostics for modules would be unfortunate, but I am also hopeful that we can bring them back in a more maintainable manner. And I would not block the removal of the feature on this regression, it seems that the maintenance burden here is high enough that we can take a small hit in functionality.

I am highly supportive of removing this code.

Thank you for the suggestion! I tried that out and was generally happy with the results (it definitely improved the modules diagnostic behavior, other behavioral changes were a bit of a wash; it did expose a crashing bug with non-delayed typo correction which I fixed). I’ve pushed the changes up on my branch.

Yeah, agreed. We should just remove it imo.

Since this got posted, I’ve started paying attention to this feature more, and am convinced that this has become an unmaintained/rotted bug-factory, and we just need to tear it out ASAP.

I realize we had two volunteers for maintenance of ALL typo correction (@tamaroning @qinkunbao), but I think we should go ahead with removing DELAYED typo correction right now. If the volunteers can work together to get a patch to re-add it with a majority of the bugs fixed, I’d be open to re-including it. BUT my understanding is that there are a reasonable amount of normal-typo correction (non-delayed) to keep them busy for a while

But at the moment, I’m convinced that its removal is an immediate and incredibly beneficial change, much more so than the benefit it gives us. This calculus obviously changes if the bugs are all fixed in a re-add patch. So lets just remove it NOW (I know Aaron is working on a patch), and be open to a ‘fixed’ version coming back in the future.

Yeah, having worked on this patch, I think removal is a good idea so we can focus efforts on improving typo correction which is less fragile.

The PR for removal can be found at: Remove delayed typo expressions by AaronBallman · Pull Request #143423 · llvm/llvm-project · GitHub

rnk June 10, 2025, 8:15pm 11

I’ll keep it short since I sense I’m piling on, but yes, we should remove this feature. It seems clear from the data that you gathered that we wouldn’t land this feature today given the instability it causes and the diagnostic improvements it creates.

If we want to improve the diagnostic experience, I think there are other higher leverage things we could be doing, like SARIF, or giving every diagnostic a short doc link with more context.