Skip to content

Stop UTILITY targets from breaking IntelliSense (#4404) - #4405

Merged
Garrett Campbell (gcampbell-msft) merged 2 commits into
microsoft:mainfrom
malsyned:issue-4404-utility-target-shadowing
Jan 22, 2026
Merged

Garrett Campbell (gcampbell-msft) merged 2 commits into
microsoft:mainfrom
malsyned:issue-4404-utility-target-shadowing

Conversation

@malsyned

Copy link
Copy Markdown
Contributor

This change addresses item #4404

This changes visible behavior

The following changes are proposed:

Avoid passing UTILITY targets to updateConfigurationData() so that they don't interfere with the assumptions made by getConfiguration().

The purpose of this change

Prevent UTILITY targets with sources, such as the ones created by Doxygen.cmake, from causing incorrect IntelliSense generation.

@malsyned

Copy link
Copy Markdown
Contributor Author

@malsyned

Dennis Lambe Jr. (malsyned) commented Apr 17, 2025 •

Copy link
Copy Markdown
Contributor Author

The strategy of this PR relies on the assumption that UTILITY targets (i.e. targets created by add_custom_target()) aren't going to contribute anything of value to C/C++ compilation information. I think that's a pretty safe assumption, and certainly safer than the assumption made in the current source very safe assumption, because CMake won't accept calls to target_include_directories() or target_compile_definitions() on UTILITY targets, so there's not really a supported way to provide those targets with the configuration information we want.

Still, if filtering them all out indiscriminately seems too risky a change to you, I can imagine a few other strategies for addressing #4404,

In increasing order of complexity:

  1. Filter out configurations that don't contribute any non-undefined, non-empty properties before returning the first remaining one.
  2. Make a new composite configuration by iterating over all target configurations and using each non-undefined, non-empty value encountered for each property
  3. Like 2, but combine array properties like includePath instead of picking the first one

I have implementations for (1) and (3) already written up, and (2) would be pretty simple to make. If you'd prefer a PR for any of those strategies, let me know and I'll push it shortly after.

@malsyned Dennis Lambe Jr. (malsyned) changed the title Stop UTILITY targets from affecting IntelliSense Stop UTILITY targets from breaking IntelliSense Apr 18, 2025
@malsyned Dennis Lambe Jr. (malsyned) changed the title Stop UTILITY targets from breaking IntelliSense Stop UTILITY targets from breaking IntelliSense (#4404) Apr 22, 2025
@malsyned

Dennis Lambe Jr. (malsyned) commented Apr 25, 2025 •

Copy link
Copy Markdown
Contributor Author

Garrett Campbell (@gcampbell-msft) I'm having a hard time seeing why this change would affect the macOS and Windows end-to-end tests but not the Linux ones. It's hard to see through all of the spurious "Error" lines about deprecation warnings and "Hello" output, but I think it's saying that the failing test is "Good Run test with coverage", is that right?

Can you give me some guidance on how to even start troubleshooting this? I don't have access to a Windows or macOS PC, and this change in the cpptools integration shouldn't be going anywhere near anything test explorer related, I don't think.

@gcampbell-msft

Garrett Campbell (gcampbell-msft) commented Apr 25, 2025 •

Copy link
Copy Markdown
Collaborator

Dennis Lambe Jr. (@malsyned) Unfortunately these are known flaky issues with our tests. The Mac has a flakiness in one test and the Windows occasionally hits a timeout. I requeued the tests.

@malsyned

Copy link
Copy Markdown
Contributor Author

Still no love from the Mac CI test, huh?

Garrett Campbell (@gcampbell-msft) does this PR look good otherwise to you?

@gcampbell-msft

Garrett Campbell (gcampbell-msft) commented Jun 11, 2025 •

Copy link
Copy Markdown
Collaborator

Dennis Lambe Jr. (@malsyned) What about an option where we simply sort the configurations we provide, and essentially de-prioritize the Utility targets? It makes me nervous to fully remove a target from intellisense inclusion, but if we simply de-prioritize it such that it's more of a last resort, this feels safer.

This is very similar to the Option 1 you mentioned, and I do think I prefer something like that, so that it can still be used if there are no other configurations available.

@malsyned

Copy link
Copy Markdown
Contributor Author

Dennis Lambe Jr. (@malsyned) What about an option where we simply sort the configurations we provide, and essentially de-prioritize the Utility targets? It makes me nervous to fully remove a target from intellisense inclusion, but if we simply de-prioritize it such that it's more of a last resort, this feels safer.

This is very similar to the Option 1 you mentioned, and I do think I prefer something like that, so that it can still be used if there are no other configurations available.

OK yeah, no problem. I can think of a couple ways to implement this, do you have a preference?

1. Explicitly look at UTILITY targets and move them to the back of the line

This has to happen at updateConfigurationData() time because target type isn't easily available at provideConfigurations() time.

diff --git a/src/cpptools.ts b/src/cpptools.ts
index 50a3a50d..34fefc19 100644
--- a/src/cpptools.ts
+++ b/src/cpptools.ts
@@ -595,7 +595,11 @@ export class CppConfigurationProvider implements cpptools.CustomConfigurationPro
             // Update only the active build type variant.
             if (config.name === opts.activeBuildTypeVariant || (!opts.activeBuildTypeVariant && config.name === "")) {
                 for (const project of config.projects) {
-                    for (const target of project.targets) {
+                    // Move utility targets to the end so executable and library
+                    // targets which contribute more to IntelliSense
+                    // configuration take precedence.
+                    const targets = project.targets.sort((a, b) => Number(a.type === 'UTILITY') - Number(b.type === 'UTILITY'));
+                    for (const target of targets) {
                         // Now some shenanigans since header files don't have config data:
                         // 1. Accumulate some "defaults" based on the set of all options for each file group
                         // 2. Pass these "defaults" down when rebuilding the config data

2. Only use non-contributing configurations as a last resort

This can happen at provideConfigurations() time. It's more like an implicit check for UTILITY targets. It might be marginally more future-proof or self documenting?

diff --git a/src/cpptools.ts b/src/cpptools.ts
index 50a3a50d..a6cfe68b 100644
--- a/src/cpptools.ts
+++ b/src/cpptools.ts
@@ -315,6 +315,27 @@ export function getIntelliSenseMode(cptVersion: cpptools.Version, compilerPath:
     }
 }
 
+/**
+ * Try to find a target configuration with some populated properties.
+ *
+ * All targets get defaults for `compilerPath`, `compilerArgs`, and
+ * `compilerFragments`, even `UTILITY` targets defined with
+ * `add_custom_command()` that provide no other useful configuration, so if
+ * possible, return one with more than just those populated.
+ */
+function fallbackConfiguration(configurations: Map<string, cpptools.SourceFileConfigurationItem> | undefined) {
+    if (!configurations) {
+        return undefined;
+    }
+    for (const item of configurations.values()) {
+        const { configuration: { includePath, defines, intelliSenseMode, standard} } = item;
+        if (includePath.length || defines.length || intelliSenseMode || standard) {
+            return item;
+        }
+    }
+    return configurations.values().next().value;
+}
+
 /**
  * The actual class that provides information to the cpptools extension. See
  * the `CustomConfigurationProvider` interface for information on how this class
@@ -350,7 +371,7 @@ export class CppConfigurationProvider implements cpptools.CustomConfigurationPro
         if (this.activeTarget && configurations?.has(this.activeTarget)) {
             return configurations!.get(this.activeTarget);
         } else {
-            return configurations?.values().next().value; // Any value is fine if the target doesn't match
+            return fallbackConfiguration(configurations);
         }
     }
 

@gcampbell-msft

Copy link
Copy Markdown
Collaborator

Dennis Lambe Jr. (@malsyned) I think I like option 2 as it's more generic, and as you said, it might be more future-proof, thanks!

@malsyned
Dennis Lambe Jr. (malsyned) force-pushed the issue-4404-utility-target-shadowing branch from 13dce1a to 7bd23b6 Compare June 12, 2025 17:32
@malsyned
Dennis Lambe Jr. (malsyned) force-pushed the issue-4404-utility-target-shadowing branch from 7bd23b6 to 7a9aead Compare June 20, 2025 20:58
@malsyned
Dennis Lambe Jr. (malsyned) force-pushed the issue-4404-utility-target-shadowing branch from 7a9aead to 5098463 Compare July 11, 2025 23:01
@malsyned

Dennis Lambe Jr. (malsyned) commented Jul 11, 2025 •

Copy link
Copy Markdown
Contributor Author

Garrett Campbell (@gcampbell-msft) just rebased this against the latest post-1.21.36 main. It uses the implementation you said you preferred, and from my end it's ready for merging. Let me know if there's anything else you want me to do on it.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving, but could you please create a new section in the CHANGELOG titled ## 1.22 and move your changelog update to there?

@malsyned

Copy link
Copy Markdown
Contributor Author

Sure. Want me to also change ## 1.21 to ## 1.21.36?

@malsyned
Dennis Lambe Jr. (malsyned) force-pushed the issue-4404-utility-target-shadowing branch from 5098463 to ada1d0c Compare August 13, 2025 03:30
@malsyned

Copy link
Copy Markdown
Contributor Author

I just noticed that #4491 got merged with its changelog update still in the 1.21 section. Want me to make a little PR to move that, once the 1.22 section exists?

@malsyned

Copy link
Copy Markdown
Contributor Author

Garrett Campbell (@gcampbell-msft) I just pushed an update that does what you asked, adding a 1.22 section and moving this change into it. LMK if you want to make these other two changes I mentioned above:

@malsyned

Copy link
Copy Markdown
Contributor Author

Garrett Campbell (@gcampbell-msft) I can't tell if the macOS CI failure is real or spurious, can you?

Some `UTILITY` targets, such as the ones created by `Doxygen.cmake`,
have a list of associated sources but don't have any useful
`includePath`, `defines`, etc. If the selected Build Target is `all` or
some other non-artifact target, these targets could be chosen as a
fallback source of configuration instead of a more complete one. Filter
out targets with empty configurations before selecting a fallback, so
that doesn't happen.
@hanniavalera
Hannia Valera (hanniavalera) force-pushed the issue-4404-utility-target-shadowing branch from c71a090 to 2e0d08b Compare January 22, 2026 21:31
@gcampbell-msft
Garrett Campbell (gcampbell-msft) merged commit 15de5c5 into microsoft:main Jan 22, 2026
4 checks passed
@malsyned
Dennis Lambe Jr. (malsyned) deleted the issue-4404-utility-target-shadowing branch February 25, 2026 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants