[Parse] Improve diagnostic and add fixit to correct '#else if' to '#elseif'

[owenv]

Resolves SR-10581

[theblixguy]

Maybe you can extract this into a separate line otherwise it makes the indentation looks a bit weird IMO.

auto diag = diag::unexpected_if_following_else_compilation_directive;
auto rangeToReplace = SourceRange(ClauseLoc, Tok.getLoc().getAdvancedLoc(1));
diagnose(Tok.getLoc(), diag).fixItReplace(rangeToReplace, "#elseif");

[owenv]

Turns out this was a little more complicated then it needed to be, it reads much better now that I've simplified it a bit

[theblixguy]

I think this is supposed to be just expected-error. Also, you can also check whether the fix-it is correctly applied by using this syntax:

{{column_start-column_end=<text>}}

so for example:

// expected-error {{unexpected 'if' keyword following '#else' conditional compilation directive; did you mean '#elseif?'}}{{0-9=#elseif}}

(wrote this without running, so you might have to tweak the range).

[beccadax]

expected-error@-1 is correct; it says the error should appear on the line before the comment.

You're correct that this needs to test the fix-it, though. Columns are 1-based (this confuses me all the time), so I think the range is 1-9.

[theblixguy]

Sorry, I meant we could use expected-error and put it on the same line as #else if BAR? I suppose it's fine otherwise because the line would become even longer 😅

[owenv]

Good catch, thanks!

[theblixguy]

cc @rintaro @brentdax (who filed this bug)

[beccadax]

Thank you for fixing this! I make this mistake all the time; it'll be nice to have the compiler point me to the right code. My suggestions are pretty minor—you've done a good job here.

[rintaro]

.getAdvancedLoc(1) shouldn't be needed.

[owenv]

fixed

[rintaro]

To maximize the recovery, I think you should do this before line 599. Like:

     bool isElse = Tok.is(tok::pound_else);
     SourceLoc ClauseLoc = consumeToken();
+    if (isElse && Tok.is(tok::kw_if) && !Tok.isAtStartOfLine()) {
+      .. Emit error and fix-it ..
+      consumeToken(tok::kw_if);
+      isElse = false;
+    }
+
     Expr *Condition = nullptr;
     bool isActive = false;

     // Parse the condition.  Evaluate it to determine the active
     // clause unless we're doing a parse-only pass.
     if (isElse) {

This way, the parser automatically parse and possibly diagnose the condition after #else if.

For example, given

#else if SOME == THING

In this case, we want to diagnose both:

• #else if for fix-it #elseif
• == for error: expected '&&' or '||' expression

[owenv]

Yeah, recovering here seems to work well and catch parse errors in the condition. Thanks!

[rintaro]

Small tips: Parser has diagnose(Token Tok, Diagnostic Diag) function. You don't need .getLoc() here.

[owenv]

fixed

[owenv]

Thanks for the feedback @theblixguy @rintaro @brentdax . I think I've addressed all the issues raised

[beccadax]

@swift-ci please smoke test

[rintaro]

Thank you!