Code fix to add unsafe option to C# project #25875

Neme12 · 2018-04-01T23:07:16Z

Fixes #25722
Fixes #23342
Partially fixes #22433 / dotnet/project-system#2732 (only for CPS projects)

~~Things I'd appreciate some feedback on:~~ #resolved

what to name this feature: UnsafeProject is obviously not great... I couldn't come up with anything better that's also not too verbose
the title: "Allow unsafe code in this project" is more in line with the VS project setting which says "Allow unsafe code", but "Add unsafe option to this project" might be somewhat more in line with what the compiler says
should we have fix all for this? I added my reasoning in a comment

…ions

Neme12 · 2018-04-01T23:18:27Z

Also note that undo does not work for this just as it doesn't work for UpgradeProject. Implementing it is not trivial because when invoking the code fix, we change options in all configurations and undoing would require only reverting the configurations that were actually changed. But this should still be tracked in an issue and fixed later.

src/Features/CSharp/Portable/CSharpFeaturesResources.resx

src/Features/CSharp/Portable/UnsafeProject/CSharpUnsafeProjectCodeFixProvider.cs

CyrusNajmabadi · 2018-04-01T23:51:16Z

src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.cs

+                        break;
+
+                    case LanguageNames.VisualBasic:
+                        throw new InvalidOperationException(ServicesVSResources.This_workspace_does_not_support_updating_Visual_Basic_compilation_options);


@jasonmalinowski can you review thei method.

src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.cs

Neme12 · 2018-04-01T23:59:17Z

src/Workspaces/Core/Portable/Workspace/Workspace.cs

-                && !this.CanApplyChange(ApplyChangesKind.ChangeCompilationOptions))
+                && !this.CanApplyChange(ApplyChangesKind.ChangeCompilationOptions)
+                && !this.CanApplyCompilationOptionChange(
+                    projectChanges.OldProject.CompilationOptions, projectChanges.NewProject.CompilationOptions, projectChanges.NewProject))
            {


Here you can see that if CanApplyChange returns false but CanApplyCompilationOptionChange returns true, this check passes. If CanApplyChange returns true, it passes regardless of what CanApplyCompilationOptionChange
returns. Therefore VisualStudioWorkspace returning true for CanApplyChange(ApplyChangesKind.ChangeParseOptions) and CanApplyChange(ApplyChangesKind.ChangeCompilationOptions) must be wrong, since it doesn't support changing arbitrary options.

@jcouv It looks like you wrote this. Could you clarify what the intent was here? Thanks

I mean the equivalent code for parse options below.

Hum, this does seem wrong. I would think that both checks should be:

if (<options changed>&& (!CanApplyChange() || !CanApplyXyzOptionChange()) ...

I'll let @jasonmalinowski confirm, since I'm not very familiar with workspaces.

In reply to: 178513336 [](ancestors = 178513336)

I'm not very familiar with workspaces

but you created this API 😄

I'll mention the two options again. This API could be interpreted in 2 ways:

CanApplyChange(ApplyChangesKind.ChangeParseOptions) returning true means the workspace is able to change some parse options - you don't know whether it can handle the option you want until you additionally call CanApplyParseOptionChange. So in this scenario, CanApplyChange really basically tells you nothing about what you can actually do. This might have been the original intent (but implemented incorrectly).

CanApplyChange(ApplyChangesKind.ChangeParseOptions) returning true means the workspace supports changing parse options, period. Any kind of change, and you can count on that. Now it may also support changing some specific options even if this returns false. If you check CanApplyParseOptionChange, you might find out that is the case. This is the existing implementation.

@jcouv I see you're suggesting we should really have the first option. I think both of them make sense, but if we go with first, then the default value of CanApplyCompilationChange being false is wrong and would be a breaking change. Basically, until now, if you get true from CanApplyChange(ApplyChangesKind.ChangeCompilationOptions) you know the workspace supports changing any compilation options, but if we implement this the way you're suggesting, the new meaning would be "no, this actually doesn't supports changing compilation options at all because CanApplyCompilationChange still always returns false". If we go with number 1, the default implementation should return true. But there's also a problem that the existing implementation of CanApplyParseChange returns false. Meh :-/

Thanks. It would make sense if this API was indeed public (and the default value of the precise check is change to true so we don't break existing implementors who use the coarse check as the only one). But I don't think I'd want to add a public API here, so then is it ok to leave that for later? What do you suggest I do about this now, leave everything as is, since things were equally broken before? Or would it not hurt this PR too much to do that here even with all the work required to review such a change?

I would either do nothing and leave all the brokeness as is, or make the API public and do the right thing. [insert Yoda meme here]

If I did do that work, do you think it would be an acceptable change to change the default value of CanApplyParseOptionChange to true? I think that's the right value because anyone who doesn't implement that and only implements CanApplyChange expects users to be able to change those options. In fact if I kept these as returning false and fixed the code checking this, it would be a much bigger breaking change.

Actually, default behavior should probably be to call CanApplyChange(ApplyChangesKind.ChangeParseOptions). That way workspaces that said no also say no from the new API, workspaces that say yes get the default "anything goes" which is the correct representation, and overriders get the right thing anyways.

OK, I was just thinking that's a little silly/redundant if the proper check is to always do CanApplyChange && CanApplyParseOptionChange anyway, but that works too I suppose. I can't think of any problems with that.

Or is the contract of this API supposed to be that CanApplyParseOptionChange is a sufficient check on its own?

Neme12 · 2018-04-02T00:03:24Z

src/Workspaces/Core/Portable/PublicAPI.Unshipped.txt

@@ -17,3 +17,4 @@ const Microsoft.CodeAnalysis.Classification.ClassificationTypeNames.ParameterNam
 const Microsoft.CodeAnalysis.Classification.ClassificationTypeNames.PropertyName = "property name" -> string
 static Microsoft.CodeAnalysis.ProjectInfo.Create(Microsoft.CodeAnalysis.ProjectId id, Microsoft.CodeAnalysis.VersionStamp version, string name, string assemblyName, string language, string filePath = null, string outputFilePath = null, Microsoft.CodeAnalysis.CompilationOptions compilationOptions = null, Microsoft.CodeAnalysis.ParseOptions parseOptions = null, System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.DocumentInfo> documents = null, System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.ProjectReference> projectReferences = null, System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.MetadataReference> metadataReferences = null, System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.Diagnostics.AnalyzerReference> analyzerReferences = null, System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.DocumentInfo> additionalDocuments = null, bool isSubmission = false, System.Type hostObjectType = null, string outputRefFilePath = null) -> Microsoft.CodeAnalysis.ProjectInfo
 static Microsoft.CodeAnalysis.ProjectInfo.Create(Microsoft.CodeAnalysis.ProjectId id, Microsoft.CodeAnalysis.VersionStamp version, string name, string assemblyName, string language, string filePath, string outputFilePath, Microsoft.CodeAnalysis.CompilationOptions compilationOptions, Microsoft.CodeAnalysis.ParseOptions parseOptions, System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.DocumentInfo> documents, System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.ProjectReference> projectReferences, System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.MetadataReference> metadataReferences, System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.Diagnostics.AnalyzerReference> analyzerReferences, System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.DocumentInfo> additionalDocuments, bool isSubmission, System.Type hostObjectType) -> Microsoft.CodeAnalysis.ProjectInfo
+virtual Microsoft.CodeAnalysis.Workspace.CanApplyCompilationOptionChange(Microsoft.CodeAnalysis.CompilationOptions oldOptions, Microsoft.CodeAnalysis.CompilationOptions newOptions, Microsoft.CodeAnalysis.Project project) -> bool


📝 New public API modeled after CanApplyParseOptionChange

Neme12 · 2018-04-02T09:38:47Z

Bad news. This doesn't work with the new project system :( AllowUnsafeBlocks throws NotImplementedExceptions, even in the getter. In fact this seems to be the case for all properties except for LanguageVersion and a few other ones.

Edit: ➡️ Fixed by using IVsBuildPropertyStorage in CPS projects (c5275a0)

Neme12 · 2018-04-02T12:13:15Z

My change in the last commit (7b511a8) looks like it fixes #23342, tagging @davkean

It still produces really verbose XML, but that's a different problem. I don't see how that could be improved with the existing ConfigurationManager API.

Neme12 · 2018-04-02T12:35:10Z

I think the problem was that ConfigurationRow returns Configurations (row is basically either Debug or Release and the individual configurations inside that row are platforms) and Item(1) was just getting the first one.

src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.cs

jaredpar · 2019-01-04T23:30:53Z

@Neme12 looks like the test DestructorOverridesNonDestructor failed. I will retry that leg to determine if this is flaky.

Note: no need to wait for me to look here. The Mono test failures show up in the Test tab just like all the other ones. 😄

jasonmalinowski · 2019-01-04T23:38:17Z

I'd say once we have a green roslyn-CI and @sharwell can confirm the roslyn-integration-CI isn't a problem, I'm OK with the merge. We may have some trouble figuring out which branch this has to be merged into though, but that's something we have to do on our side.

Neme12 · 2019-01-05T19:26:46Z

@sharwell Do you know why the integration tests are failing? Is it safe to ignore?

Neme12 · 2019-01-11T09:57:27Z

@jasonmalinowski There was a green Windows_VisualStudio_Integration_Tests release run now. Debug timed out, but not because of my tests. If you look at the tests tab, show tests with all outcomes and search for "CPSProject" and "LegacyProject", you can see that all 6 of them passed. Are you happy with merging now?

Neme12 · 2019-01-14T18:33:27Z

@jasonmalinowski Is there anything preventing this from going in?

jasonmalinowski · 2019-01-14T18:36:25Z

@sharwell Can you confirm this looks good from your perspective?

jasonmalinowski · 2019-01-14T18:37:11Z

(and @sharwell by that I just mean whether the integration tests meet expectations)

jasonmalinowski · 2019-01-16T05:16:01Z

@Neme12 You're OK if I push to your branch to resolve the merge conflict?

Neme12 · 2019-01-16T05:52:11Z

@jasonmalinowski Yes, feel free to do that. Thanks.

Neme12 · 2019-01-16T20:50:38Z

@jasonmalinowski Can you please rerun Linux_Test coreclr?

jasonmalinowski · 2019-01-16T22:09:56Z

@sharwell confirmed privately that the integration test issues seem unrelated.

Neme12 · 2019-01-16T22:11:47Z

Thanks. 🎉 Finally I get some closure here 😄

CyrusNajmabadi · 2019-01-16T22:12:42Z

Congrats! I actually ran into this a couple of days ago as part of a discussion around fixed size buffers. And i was sad this didn't work!

Neme12 · 2019-01-16T22:14:26Z

Me too!

CyrusNajmabadi · 2019-01-16T22:14:38Z

Finally I get some closure here 😄

Talk to @YairHalberstadt. He has some good ideas on how to deal with closures.

By saying we support ChangeCompilationOptions and ChangeParseOptions, this meant that we supported _all_ possible changes; if you changed an option in a code fix we didn't actually support updating your project file for, we'd still let the code fix apply and just not update your project file. That's not terribly great. Returning false for CanApplyChanges then means the workspace calls the more precise methods that actually check the specific option. I think this means all the specific checking done in dotnet#25875 never actually worked.

Neme12 added 9 commits April 1, 2018 18:29

Allowing ChangeCompilationOptions in VisualStudioWorkspace

8c35eaf

This_workspace_does_not_support_updating_Visual_Basic_compilation_opt…

5f8c43d

…ions

CanApplyCompilationOptionChange

6db55aa

Adding AllowUnsafe option to TestWorkspace

ad48331

Simplifying CreateCompilationOptionsElement

8cdfeda

Adding tests + implementation

dbfbdbe

Renaming ParseOptionsChangeAction

61001fb

Localization

cbd1e91

Adding comment for missing fix all

f162695

Neme12 requested a review from a team as a code owner April 1, 2018 23:07

CyrusNajmabadi reviewed Apr 1, 2018

View reviewed changes

src/Features/CSharp/Portable/CSharpFeaturesResources.resx Show resolved Hide resolved

CyrusNajmabadi reviewed Apr 1, 2018

View reviewed changes

src/Features/CSharp/Portable/UnsafeProject/CSharpUnsafeProjectCodeFixProvider.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Apr 1, 2018

View reviewed changes

Neme12 commented Apr 1, 2018

View reviewed changes

src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.cs Show resolved Hide resolved

Neme12 commented Apr 1, 2018

View reviewed changes

Neme12 commented Apr 2, 2018

View reviewed changes

Neme12 mentioned this pull request Apr 2, 2018

Add CSharp and VB Project Configuration Properties for Automation dotnet/project-system#2172

Merged

Simplifying enumeration of ConfigurationManager

7b511a8

Neme12 mentioned this pull request Apr 2, 2018

UpgradeProject crashes IDE with "Unable to check out the files from source control" #18199

Closed

Neme12 mentioned this pull request Apr 2, 2018

Upgrade language version fixer fails to update the correct program configuration #23342

Closed

jcouv added the Area-IDE label Apr 2, 2018

jcouv added this to the 15.8 milestone Apr 2, 2018

jcouv reviewed Apr 2, 2018

View reviewed changes

src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.cs Outdated Show resolved Hide resolved

Neme12 added 2 commits April 3, 2018 00:11

Renaming

a934f22

Updating namespaces

6fd37d0

Neme12 closed this Jan 11, 2019

Neme12 reopened this Jan 11, 2019

jasonmalinowski changed the base branch from master to dev16.1-preview1 January 15, 2019 22:21

Merge remote-tracking branch 'dotnet/dev16.1-preview1' into allowUnsafe

0edab52

jasonmalinowski merged commit b8b7d7e into dotnet:dev16.1-preview1 Jan 16, 2019

Neme12 deleted the allowUnsafe branch January 16, 2019 22:10

jcouv modified the milestones: 16.0, 16.1.P1 Jan 16, 2019

kzu mentioned this pull request Apr 1, 2019

Allow code fixes to modify the project file #34636

Open

This was referenced Jun 17, 2020

Fix VisualStudioWorkspaceImpl.CanApplyChange around options #45243

Open

Document and test how CanApplyParse/CompilationOptions works #45266

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code fix to add unsafe option to C# project #25875

Code fix to add unsafe option to C# project #25875

Neme12 commented Apr 1, 2018 •

edited

Loading

Neme12 commented Apr 1, 2018 •

edited

Loading

CyrusNajmabadi Apr 1, 2018

Neme12 Apr 1, 2018 •

edited

Loading

Neme12 Apr 2, 2018

Neme12 Apr 2, 2018

jcouv Apr 2, 2018

Neme12 Apr 2, 2018 •

edited

Loading

Neme12 Apr 4, 2018 •

edited

Loading

jasonmalinowski Apr 4, 2018

Neme12 Apr 4, 2018 •

edited

Loading

jasonmalinowski Apr 5, 2018

Neme12 Apr 5, 2018 •

edited

Loading

Neme12 Apr 2, 2018 •

edited

Loading

Neme12 commented Apr 2, 2018 •

edited

Loading

Neme12 commented Apr 2, 2018 •

edited

Loading

Neme12 commented Apr 2, 2018 •

edited

Loading

jaredpar commented Jan 4, 2019

jasonmalinowski commented Jan 4, 2019

Neme12 commented Jan 5, 2019

Neme12 commented Jan 11, 2019

Neme12 commented Jan 14, 2019

jasonmalinowski commented Jan 14, 2019

jasonmalinowski commented Jan 14, 2019

jasonmalinowski commented Jan 16, 2019

Neme12 commented Jan 16, 2019

Neme12 commented Jan 16, 2019

jasonmalinowski commented Jan 16, 2019

Neme12 commented Jan 16, 2019

CyrusNajmabadi commented Jan 16, 2019

Neme12 commented Jan 16, 2019

CyrusNajmabadi commented Jan 16, 2019

Code fix to add unsafe option to C# project #25875

Code fix to add unsafe option to C# project #25875

Conversation

Neme12 commented Apr 1, 2018 • edited Loading

Neme12 commented Apr 1, 2018 • edited Loading

CyrusNajmabadi Apr 1, 2018

Choose a reason for hiding this comment

Neme12 Apr 1, 2018 • edited Loading

Choose a reason for hiding this comment

Neme12 Apr 2, 2018

Choose a reason for hiding this comment

Neme12 Apr 2, 2018

Choose a reason for hiding this comment

jcouv Apr 2, 2018

Choose a reason for hiding this comment

Neme12 Apr 2, 2018 • edited Loading

Choose a reason for hiding this comment

Neme12 Apr 4, 2018 • edited Loading

Choose a reason for hiding this comment

jasonmalinowski Apr 4, 2018

Choose a reason for hiding this comment

Neme12 Apr 4, 2018 • edited Loading

Choose a reason for hiding this comment

jasonmalinowski Apr 5, 2018

Choose a reason for hiding this comment

Neme12 Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

Neme12 Apr 2, 2018 • edited Loading

Choose a reason for hiding this comment

Neme12 commented Apr 2, 2018 • edited Loading

Neme12 commented Apr 2, 2018 • edited Loading

Neme12 commented Apr 2, 2018 • edited Loading

jaredpar commented Jan 4, 2019

jasonmalinowski commented Jan 4, 2019

Neme12 commented Jan 5, 2019

Neme12 commented Jan 11, 2019

Neme12 commented Jan 14, 2019

jasonmalinowski commented Jan 14, 2019

jasonmalinowski commented Jan 14, 2019

jasonmalinowski commented Jan 16, 2019

Neme12 commented Jan 16, 2019

Neme12 commented Jan 16, 2019

jasonmalinowski commented Jan 16, 2019

Neme12 commented Jan 16, 2019

CyrusNajmabadi commented Jan 16, 2019

Neme12 commented Jan 16, 2019

CyrusNajmabadi commented Jan 16, 2019

Neme12 commented Apr 1, 2018 •

edited

Loading

Neme12 commented Apr 1, 2018 •

edited

Loading

Neme12 Apr 1, 2018 •

edited

Loading

Neme12 Apr 2, 2018 •

edited

Loading

Neme12 Apr 4, 2018 •

edited

Loading

Neme12 Apr 4, 2018 •

edited

Loading

Neme12 Apr 5, 2018 •

edited

Loading

Neme12 Apr 2, 2018 •

edited

Loading

Neme12 commented Apr 2, 2018 •

edited

Loading

Neme12 commented Apr 2, 2018 •

edited

Loading

Neme12 commented Apr 2, 2018 •

edited

Loading