Skip to content
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

Support Compiler Argument Trait AB Experiment #12979

Merged
merged 1 commit into from
Dec 2, 2024

Conversation

kuchungmsft
Copy link
Contributor

@kuchungmsft kuchungmsft commented Nov 19, 2024

  • Depends on cpptools' update to provide ProjectContextResult.
  • Send "standardVersion" trait in completion prompt by default.
  • Added the following new traits
    • intelliSenseDisclaimer: compiler information disclaimer.
    • intelliSenseDisclaimerBeginning: to note the beginning of IntelliSense information.
    • compilerArguments: a list of compiler command arguments that could affect Copilot generating completions.
    • directAsks: direct asking Copilot to do something instead of providing an argument.
    • intelliSenseDisclaimerEnd: to note the end of IntelliSense information.
  • A/B Experimental flags
    • copilotcppTraits: deprecated, no longer used.
    • copilotcppExcludeTraits:: deprecated, no longer used.
    • copilotcppIncludeTraits: string array to include individual trait, i.e., compilerArguments.
    • copilotcppMsvcCompilerArgumentFilter: map of regex string to absence prompt for MSVC.
    • copilotcppClangCompilerArgumentFilter: map of regex string to absence prompt for Clang.
    • copilotcppGccCompilerArgumentFilter: map of regex string to absence prompt for GCC.
    • copilotcppCompilerArgumentDirectAskMap: map of argument to prompt.

@kuchungmsft kuchungmsft changed the title Support A/B Compiler Arguments Traits Support Compiler Argument Trait AB Experiment Nov 19, 2024
kuchungmsft added a commit to kuchungmsft/vscode-cpptools that referenced this pull request Nov 19, 2024
- Depends on microsoft#12979 to merge first.
- copilotcppMacroReferenceFilter: regex string to filter macro reference for telemetry.
- Telemetry related to user defines and macro references.
- Added new trait compilerUserDefines to note the relevant user defines to the editing file.
@kuchungmsft kuchungmsft marked this pull request as ready for review November 19, 2024 23:51
@kuchungmsft kuchungmsft force-pushed the kuchung/SupportCompilerArgumentAB branch from 92e251b to ce1c8fe Compare November 20, 2024 00:41
sean-mcmanus
sean-mcmanus previously approved these changes Nov 20, 2024
Copy link
Contributor

@sean-mcmanus sean-mcmanus left a comment

Choose a reason for hiding this comment

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

I'm approving, but I haven't been able to test it out and you should probably get another approver.

@benmcmorran
Copy link
Member

Pulling this down to test locally now and then I expect to be able to sign off.

@kuchungmsft kuchungmsft force-pushed the kuchung/SupportCompilerArgumentAB branch from ce1c8fe to 7a72866 Compare November 22, 2024 00:40
@kuchungmsft kuchungmsft requested a review from a team as a code owner November 22, 2024 00:40
benmcmorran
benmcmorran previously approved these changes Nov 22, 2024
Extension/src/LanguageServer/lmTool.ts Outdated Show resolved Hide resolved
Extension/src/telemetry.ts Outdated Show resolved Hide resolved
Extension/src/LanguageServer/utils.ts Outdated Show resolved Hide resolved
sean-mcmanus
sean-mcmanus previously approved these changes Nov 22, 2024
Copy link
Contributor

@sean-mcmanus sean-mcmanus left a comment

Choose a reason for hiding this comment

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

I left some comments, but I don't know if they need to be addressed or not.

@sean-mcmanus
Copy link
Contributor

@benmcmorran Were you done reviewing? I wanted to get this in on Monday.

benmcmorran
benmcmorran previously approved these changes Nov 23, 2024
@benmcmorran
Copy link
Member

@sean-mcmanus Yes, changes look good to me.

@kuchungmsft
Copy link
Contributor Author

@sean-mcmanus I need to make some changes for the following requirements.

  • If none of the MSVC /EHxx is provided, it means that exception handling is disabled. We need to be able to provide a prompt text to Copilot in such case.
  • A/B experiments cannot share feature flags, I need to make it so that compiler argument experiment won't be affected by the feature flags(copilotcppExcludeTraits, copilotcppTraits) used in language standard trait experiment.

@sean-mcmanus
Copy link
Contributor

  • If none of the MSVC /EHxx is provided, it means that exception handling is disabled. We need to be able to provide a prompt text to Copilot in such case.

@kuchungmsft I don't understand -- exception handling is enabled by default with MSVC (/EHsc is the default).

@kuchungmsft
Copy link
Contributor Author

  • If none of the MSVC /EHxx is provided, it means that exception handling is disabled. We need to be able to provide a prompt text to Copilot in such case.

@kuchungmsft I don't understand -- exception handling is enabled by default with MSVC (/EHsc is the default).

If you don't specify any /EH arguments, it's disabled, see the following.
image

@kuchungmsft kuchungmsft force-pushed the kuchung/SupportCompilerArgumentAB branch from a1261e5 to 50c3e58 Compare November 26, 2024 23:01
@kuchungmsft kuchungmsft force-pushed the kuchung/SupportCompilerArgumentAB branch from 50c3e58 to c949ac6 Compare November 26, 2024 23:12
sean-mcmanus
sean-mcmanus previously approved these changes Nov 27, 2024
@sean-mcmanus
Copy link
Contributor

sean-mcmanus commented Nov 27, 2024

@kuchungmsft FYI, clang-cl.exe isn't correctly handled -- the compiler is reported as Clang, but the args it excepts are normally msvc args like /DFOO=1 instead of -DFOO=1, but it also accepts clang args via -Xclang <arg>...I'm not sure what priority that is, but it seems like that could be dealt with in a potential follow up.

Oh....but we're not even telling Copilot that clang-cl is being used...hmm...?

@kuchungmsft
Copy link
Contributor Author

@kuchungmsft FYI, clang-cl.exe isn't correctly handled -- the compiler is reported as Clang, but the args it excepts are normally msvc args like /DFOO=1 instead of -DFOO=1, but it also accepts clang args via -Xclang <arg>...I'm not sure what priority that is, but it seems like that could be dealt with in a potential follow up.

Oh....but we're not even telling Copilot that clang-cl is being used...hmm...?

We can have regex strings to filter "/GR-", "-GR-", "-fno-rtti" for Clang compiler and map those arguments to prompt text like "Do not use dynamic_cast". We'll AB with prompt text first without raw argument values.

Extension/src/LanguageServer/copilotProviders.ts Outdated Show resolved Hide resolved
Extension/src/LanguageServer/copilotProviders.ts Outdated Show resolved Hide resolved
Extension/src/LanguageServer/lmTool.ts Outdated Show resolved Hide resolved
Extension/src/LanguageServer/lmTool.ts Outdated Show resolved Hide resolved
@kuchungmsft kuchungmsft force-pushed the kuchung/SupportCompilerArgumentAB branch 2 times, most recently from 73d1d73 to 1182714 Compare December 2, 2024 18:19
- Depends on cpptools' update to provide ProjectContextResult.
- Send "standardVersion" trait in completion prompt by default.
- Added the following new traits
  - intelliSenseDisclaimer: compiler information disclaimer.
  - intelliSenseDisclaimerBeginning: to note the beginning of IntelliSense information.
  - compilerArguments: a list of compiler command arguments that could affect Copilot generating completions.
  - directAsks: direct asking Copilot to do something instead of providing an argument.
  - intelliSenseDisclaimerEnd: to note the end of IntelliSense information.
- A/B Experimental flags
  - copilotcppTraits: deprecated, no longer used.
  - copilotcppExcludeTraits:: deprecated, no longer used.
  - copilotcppIncludeTraits: string array to include individual trait, i.e., compilerArguments.
  - copilotcppMsvcCompilerArgumentFilter: map of regex string to absence prompt for MSVC.
  - copilotcppClangCompilerArgumentFilter: map of regex string to absence prompt for Clang.
  - copilotcppGccCompilerArgumentFilter: map of regex string to absence prompt for GCC.
  - copilotcppCompilerArgumentDirectAskMap: map of argument to prompt.
@kuchungmsft kuchungmsft force-pushed the kuchung/SupportCompilerArgumentAB branch from 1182714 to 42cd504 Compare December 2, 2024 21:38
@kuchungmsft kuchungmsft merged commit a6089ea into main Dec 2, 2024
6 checks passed
@kuchungmsft kuchungmsft deleted the kuchung/SupportCompilerArgumentAB branch December 2, 2024 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants