[clangd] Dont mark terminating PP-directives as skipped

[kadircet]

Summary

PP-callback includes the terminating PP-directive
(else/elifdef/elifndef/endif) in the skipped source range. This results in
confusion as in theory that PP-directive is not skipped.
This patch changes the end location to be start of the line containing the
termination directive. That way clangd can keep highlightings for macro names in
the terminating directive as well.
This patch doesn't change the semantics of the PP-callback, as the range
possibly contains comments etc. trailing the termination directive. It's unclear
how useful that's for applications (only coverage mapping makes use of the full
range, rest always uses the endifloc for termination location), but it
definitely looks like a more intrusive change than just handling in clangd.

[hokein]

sorry, I might be lack of the context, where is the user complaint? I'm not sure which cases are improved with this patch.

Ideally we would not mark PP directives as inactive regions, but we never do that (FIXME), I think we're trying to fix that?

clang-tools-extra/clangd/CollectMacros.h

90

This looks like a semantic-highlight-specific change, instead of doing it here, would it make more sense to do it in the SemanticHighlighting.cpp?

clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp

447

It seems to me that the new behavior of these cases is worse -- #ifedf will not be highlighted while the paired #endif will, this inconsistency probably gives weird and confusing UI experience to users.

I think it is important to have a consistent decision -- highlight both or not-highlight both.

[kadircet]

In D125863#3522025, @hokein wrote:

sorry, I might be lack of the context, where is the user complaint? I'm not sure which cases are improved with this patch.

so the complaint was from an example with an active branch, eg:

#if 1
void foo();
#else
#endif

The user was complaining that #endif was inactive depending on first or second branch being active. Hence my initial thoughts were also around cases which had at least one "active" branch, but you're right this actually is confusing when there are no active branches.

Ideally we would not mark PP directives as inactive regions, but we never do that (FIXME), I think we're trying to fix that?

Right, I wasn't aware of that fix me. I was deliberately only excluding the termination directive from the skipped range and not the beginning, as I think it's nice to directly observe the fact that the condition evaluated to false/skipped.
But as mentioned above, i think it's confusing and in theory a "lie". Because preprocessor is definitely not skipping those directives, they're still processed (even in the cases where we have else/elif directives that come after an active branch).
So I suppose the right thing here is actually to preserve both start and termination directive for each block, to be consistent and also to give the correct semantics around "this PP-directive wasn't skipped".
WDYT?

clang-tools-extra/clangd/CollectMacros.h

90

I wanted to perform the change here, because other consumers of skipped ranges should actually behave similar in clangd (not that we have more ATM).
We're definitely collecting information about PP-directives that start/terminate those skipped blocks, and special casing them in some places while not in others will probably result in bugs.

Do you see any reasons that we might actually want skipped ranges to include starting/terminating directives?

clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp

447

agreed see my main comment.

[hokein]

In D125863#3522384, @kadircet wrote:

In D125863#3522025, @hokein wrote:

sorry, I might be lack of the context, where is the user complaint? I'm not sure which cases are improved with this patch.

so the complaint was from an example with an active branch, eg:

#if 1
void foo();
#else
#endif

The user was complaining that #endif was inactive depending on first or second branch being active. Hence my initial thoughts were also around cases which had at least one "active" branch, but you're right this actually is confusing when there are no active branches.

Thanks. +1, yeah, I agree that this case is confusing.

Ideally we would not mark PP directives as inactive regions, but we never do that (FIXME), I think we're trying to fix that?

Right, I wasn't aware of that fix me. I was deliberately only excluding the termination directive from the skipped range and not the beginning, as I think it's nice to directly observe the fact that the condition evaluated to false/skipped.
But as mentioned above, i think it's confusing and in theory a "lie". Because preprocessor is definitely not skipping those directives, they're still processed (even in the cases where we have else/elif directives that come after an active branch).
So I suppose the right thing here is actually to preserve both start and termination directive for each block, to be consistent and also to give the correct semantics around "this PP-directive wasn't skipped".
WDYT?

Yeah, this sounds good to me.

clang-tools-extra/clangd/CollectMacros.h

90

I don't have a strong rationale for this -- my feeling is that the CollectMacro class is the common data structure whose main responsibility is to collect information from the PPCallback, we'd better not to put specific-logic in that (otherwise skipped range concept is subtly different in CollectMacro/PPCallback, I think it might cause confusion).

Actually I think either can work as these skipped ranges are only used in semantic highlighting case. Maybe you're right -- we might want it for all cases. So up to you (if we do it here, we need to update the document for MainFileMacros::SkippedRanges).

[nridge]

Potentially related issue: https://github.com/clangd/clangd/issues/773

[daiyousei-qz]

I have a similar change recently presented in https://github.com/clangd/clangd/issues/1181 that makes all conditional preprocessor lines active. Please refer to the change if that's helpful :)

[nridge]

I believe this was superseded by D151190, should we close this?

clang-tools-extra/clangd/CollectMacros.h

void Defined(const Token &MacroName, const MacroDefinition &MD,
                SourceRange Range) override {
     add(MacroName, MD.getMacroInfo());
   }
 
   void SourceRangeSkipped(SourceRange R, SourceLocation EndifLoc) override {
     if (!InMainFile)
       return;
     Position Begin = sourceLocToPosition(SM, R.getBegin());
-    Position End = sourceLocToPosition(SM, R.getEnd());
+    Position End = sourceLocToPosition(SM, EndifLoc);
+    // Don't mark the terminating PP-directive as skipped.
+    End.character = 0;
     Out.SkippedRanges.push_back(Range{Begin, End});
   }
 
 private:
   void add(const Token &MacroNameTok, const MacroInfo *MI,
            bool IsDefinition = false);
   const SourceManager &SM;
   bool InMainFile = true;

clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp

)cpp",
       R"cpp(
       #ifndef $Macro[[name]]
       #define $Macro_decl[[name]]
       #endif
 
       #define $Macro_decl[[test]]
       #undef $Macro[[test]]
 $InactiveCode[[#ifdef test]]
-$InactiveCode[[#endif]]
+#endif
 
 $InactiveCode[[#if defined(test)]]
-$InactiveCode[[#endif]]
+#endif
     )cpp",
       R"cpp(
       struct $Class_decl[[S]] {
         float $Field_decl[[Value]];
         $Class[[S]] *$Field_decl[[Next]];
       };
       $Class[[S]] $Variable_decl[[Global]][2] = {$Class[[S]](), $Class[[S]]()};
       auto [$Variable_decl[[G1]], $Variable_decl[[G2]]] = $Variable[[Global]];
)cpp",
         $Class[[Foo]]<$TemplateParameter[[TT]], $TemplateParameter[[TTs]]...>
           *$Field_decl[[t]];
       };
     )cpp",
       // Inactive code highlighting
       R"cpp(
       // Code in the preamble.
       // Inactive lines get an empty InactiveCode token at the beginning.
-$InactiveCode[[#ifdef test]]
-$InactiveCode[[#endif]]
+$InactiveCode[[#ifdef test // With comment]]
+#endif
 
       // A declaration to cause the preamble to end.
       int $Variable_decl[[EndPreamble]];
 
       // Code after the preamble.
       // Code inside inactive blocks does not get regular highlightings
       // because it's not part of the AST.
       #define $Macro_decl[[test2]]
 $InactiveCode[[#if defined(test)]]
 $InactiveCode[[int Inactive2;]]
-$InactiveCode[[#elif defined(test2)]]
+#elif defined($Macro[[test2]])
       int $Variable_decl[[Active1]];
 $InactiveCode[[#else]]
 $InactiveCode[[int Inactive3;]]
-$InactiveCode[[#endif]]
+#endif
 
       #ifndef $Macro[[test]]
       int $Variable_decl[[Active2]];
       #endif
 
 $InactiveCode[[#ifdef test]]
 $InactiveCode[[int Inactive4;]]
-$InactiveCode[[#else]]
+#else
       int $Variable_decl[[Active3]];
       #endif
     )cpp",
       // Argument to 'sizeof...'
       R"cpp(
       template <typename... $TemplateParameter_decl[[Elements]]>
       struct $Class_decl[[TupleSize]] {
         static const int $StaticField_decl_readonly_static[[size]] =