Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

malsyned
Copy link
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
Contributor Author

malsyned commented Apr 15, 2025 via email

@malsyned malsyned force-pushed the issue-4404-utility-target-shadowing branch from 991aebb to 7de05e1 Compare April 17, 2025 15:17
@malsyned
Copy link
Contributor Author

malsyned commented Apr 17, 2025

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 malsyned changed the title Stop UTILITY targets from affecting IntelliSense Stop UTILITY targets from breaking IntelliSense Apr 18, 2025
@malsyned malsyned changed the title Stop UTILITY targets from breaking IntelliSense Stop UTILITY targets from breaking IntelliSense (#4404) Apr 22, 2025
@malsyned
Copy link
Contributor Author

malsyned commented Apr 25, 2025

@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
Copy link
Collaborator

gcampbell-msft commented Apr 25, 2025

@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
Contributor Author

Still no love from the Mac CI test, huh?

@gcampbell-msft does this PR look good otherwise to you?

@gcampbell-msft
Copy link
Collaborator

gcampbell-msft commented Jun 11, 2025

@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.

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.
@malsyned
Copy link
Contributor Author

@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
Collaborator

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

@malsyned malsyned force-pushed the issue-4404-utility-target-shadowing branch from 13dce1a to 7bd23b6 Compare June 12, 2025 17:32
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.

2 participants