Skip to content
This repository has been archived by the owner on Oct 4, 2021. It is now read-only.

Support multi-target framework intellisense #7848

Merged
merged 37 commits into from Aug 8, 2019
Merged

Support multi-target framework intellisense #7848

merged 37 commits into from Aug 8, 2019

Conversation

mrward
Copy link
Member

@mrward mrward commented Jun 10, 2019 •
edited

Type system service now supports multi-target frameworks.
Path bar supports multi-target framework selection.

MultiTargetFrameworkDropdown

MultiTargetIntellisense

Framework execution targets shown in main toolbar.

ExecutionTarget

SelectFrameworkExecutionTarget

Fixes VSTS #572311 - Multi-framework support in Roslyn workspace
Fixes VSTS #572342 - Support picking target framework for intellisense
Fixes VSTS #937105 Show (Multiple TargetFrameworks) in project options
Fixes VSTS #572309 - Execution target support for multi-framework projects
Fixes VSTS #935387 - System.NullReferenceException exception in MonoDevelop.Projects.DotNetProject.d__XXX.MoveNext() - Transitive references null reference exception fixed since this code no longer existx.
Fixes VSTS #572307 - Ensure navigation/refactoring operations on multi-frameworks projects are coalesced
Fixes VSTS #843603 Files with build action 'None' are still used by Intellisense

  • Cache framework specific configuration
    • Ensure the cache is refreshed when project options page changes config settings.
  • OnExecute project extension should be framework aware
    - Do not want ASP.NET Core project extension checking for certificate status when .NET Framework is the execution target.
  • Text editor path bar does not update after changing from a single framework to multiple.
    • See type system errors after saving the edited .csproj - Gist
    • Changing frameworks in .csproj does not always update the path bar. Sometimes see no owner projects, sometimes no workspace changed event is fired.
      • After the project file reload the OpenDocumentReferences stored in the type system have an associated Project but its ParentSolution is null. This results in TryOpenDocumentInWorkspace doing nothing which presumably then breaks the editor path bar.

@mrward mrward requested a review from a team June 10, 2019 10:06
slluis
slluis reviewed Jun 10, 2019
View reviewed changes
main/src/core/MonoDevelop.Core/MonoDevelop.Projects/ItemFrameworkConfigurationSelector.cs Outdated
var copy = new DotNetProjectConfiguration (name, platform, framework);
copy.CopyFrom (itemConfiguration);

return copy;
Copy link
Member

Choose a reason for hiding this comment

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

That is weird. A selector is supposed to return one of the configurations in the provided target, not create a new one. Why is this required?

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly this is a bit of a hack which I do not like.

The framework information is needed when running an MSBuild target so I was trying to get this to the DotNetProject when it runs MSBuild to get references etc without adding an extra framework parameter to each method. Currently the active selector in the workbench has no knowledge of the framework so the type system service is cheating here and creates another selector and sets the framework it wants.

slluis
slluis reviewed Jun 10, 2019
View reviewed changes
main/src/core/MonoDevelop.Core/MonoDevelop.Projects/ItemConfiguration.cs Outdated
}
}

public string Platform {
get { return platform ?? string.Empty; }
}

public string Framework {
get { return framework; }
Copy link
Member

Choose a reason for hiding this comment

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

Isn't the concept of framework is specific to .net? in that case this should be part of DotNetProjectConfiguration.

@mrward mrward force-pushed the multi-target-framework-type-system branch from 8758e71 to e756d6b Compare June 20, 2019 17:13
@mrward mrward force-pushed the multi-target-framework-type-system branch 2 times, most recently from ee4183b to 04e2798 Compare July 9, 2019 11:36
@mrward mrward force-pushed the multi-target-framework-type-system branch 3 times, most recently from 501ba8d to 9dc8ae5 Compare July 16, 2019 17:50
@mrward mrward changed the title [WIP] Support multi-target framework intellisense Support multi-target framework intellisense Jul 16, 2019
@mrward mrward requested a review from a team July 18, 2019 13:35
Therzok
Therzok reviewed Jul 19, 2019
View reviewed changes
main/src/core/MonoDevelop.Core/MonoDevelop.Projects/SdkProjectExtension.cs
var traversedProjects = new HashSet<string> (StringComparer.OrdinalIgnoreCase);
traversedProjects.Add (Project.ItemId);
string include = MSBuildProjectService.ToMSBuildPath (Project.ItemDirectory, fileName);
var globItems = Project.MSBuildProject.FindGlobItemsIncludingFile (include).ToList ();
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can do:

Suggested change
var globItems = Project.MSBuildProject.FindGlobItemsIncludingFile (include).ToList ();
var globItems = Project.MSBuildProject.FindGlobItemsIncludingFile (include).SingleOrDefault ();

To avoid matching all the items.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure I can change this. There may not be any matches - but could use FirstOrDefault instead. If there is more than one match we do not know what to return so fallback to asking the project for its default build action. Although not sure I have seen any examples of multiple matches here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, was just trying to avoid a new collection allocation, as it seems redundant.

@Therzok
Copy link
Contributor

Therzok commented Jul 19, 2019

LGTM!

@mrward
[Ide] Fix open documents referencing disposed project
9ac081b 
When a project file is edited in the text editor and saved, triggering
a reload, the open documents registered with the type system would
not update their owner. This would happen if the document was
referenced by more than one text editor extension - the owner changing
would not remove the open document when the DocumentRegistration
class was disposed since another reference kept it alive. This
resulted in the C# path bar not being updated for multi-target
framework projects when the frameworks were changed and the project
file saved.

To fix this the owner of the open document is updated if it is not
null and does not match the original owner. This allows the type
system's TryRegisterOpenDocument to update the Roslyn workspace and
trigger the workspace changed event used by the C# path bar.
@mrward
[Core] Cache framework specific configurations
471c556 
These get used fairly frequently by the type system service and
without being cached resulted in the MSBuild properties for the
project being evaluated each time.
@mrward
[Core] Make project configuration's framework short name internal
6fe7b85 
Renamed the property from Framework to FrameworkShortName since there
is an existing TargetFramework property.

Make FrameworkShortName property internal. The TargetFramework
property is public and can be used instead.
@mrward
[DotNetCore] Test node builders
5d22649 
Refactored the dependency node tests so they test the node builders
themselves. Previously the data object classes were being tested.
This is in preparation for new tests for multi-target projects.
@mrward
[DotNetCore] Show multi-target framework nodes as parent nodes
125d29d 
Previously multi-target framework projects would show the framework
nodes under Dependencies - NuGet and Dependencies - SDK. Now the
framework nows appear directly under the Dependencies folder. Note
that currently the Assemblies and Projects folders do not support
multi-target frameworks and just show all assemblies and projects
referenced directly in the project ignoring any conditions.
@mrward
[Core] Remove duplicate framework short name property from config
a08a5df 
A different feature branch added TargetFrameworkShortName to the
DotNetProjectConfiguration. Re-use the same property instead of
having a different property for use with multi-target projects.
@mrward
[CSharpBinding] Refactor OwnerProjectInfo
5cfca8b 
Update code after review. Seal class and implement IEquatable.
@mrward
[DotNetCore] Refactor node builder
d0fb20d 
Remove local variable in node builder after code review. The local
variable is not needed in the method.
@mrward
[Core] Improvements following review
9db1f8e 
Avoid allocations by using a value tuple as the key for the
framework specific configurations.

DotNetProject's OnClearCachedData does not need to be async.

Specify list length when creating the execution target list since it
is known.
@mrward
[Ide] Address review comments
bf0b218 
Fixed some code duplication.
Make some fields readonly.
Specify TaskScheduler for ContinueWith.
Avoid use of LINQ.
@mrward
[NuGet] Move framework icon to Ide
02fbc07 
The Framework icon is used as the image for an execution target if
it is a framework belonging to a multi-target framework project.
@mrward
[Ide] Fix C# files with BuildAction of None used for code completion
313d22a 
The type system service was only checking the file extension not
the build action.

Fixes VSTS #843603 - Files with build action 'None' are still used by
Intellisense
@mrward
[Core] Make part of Project API internal not protected
ca48c96 
Project.CreateProjectInstanceForConfigurationAsync does not need to
be protected since it is only used with MonoDevelop.Core.
@mrward mrward force-pushed the multi-target-framework-type-system branch from 6fd0ba0 to ca48c96 Compare August 7, 2019 09:02
@mrward mrward added this to the 8.3 milestone Aug 7, 2019
@mrward mrward requested review from Therzok and slluis August 7, 2019 09:03
@Therzok Therzok added the verified The feature or bug fix has been tested and it works label Aug 7, 2019
@Therzok
Copy link
Contributor

Therzok commented Aug 7, 2019

Took it for a spin, works properly from what I've tested! Nice work!

@sgmunn
Copy link
Member

sgmunn commented Aug 7, 2019

UI test for project system is from Nuget changes and is fixed in master.
.Net core tests are from mono bug.

Neither failings are related as far as we know.

sgmunn
sgmunn approved these changes Aug 7, 2019
View reviewed changes
Copy link
Member

@sgmunn sgmunn left a comment

Choose a reason for hiding this comment

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

works good

@slluis slluis merged commit ae85672 into master Aug 8, 2019
@slluis slluis deleted the multi-target-framework-type-system branch August 8, 2019 09:36
This was referenced Aug 8, 2019
Closed
Closed
Closed
Closed
@DavidKarlas DavidKarlas mentioned this pull request Oct 10, 2019
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Therzok Therzok Therzok approved these changes

@slluis slluis slluis approved these changes

@sgmunn sgmunn sgmunn approved these changes

@DavidKarlas DavidKarlas Awaiting requested review from DavidKarlas

@mkrueger mkrueger Awaiting requested review from mkrueger

Assignees
No one assigned
Labels
verified The feature or bug fix has been tested and it works
Projects
None yet
Milestone
8.3
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mrward @Therzok @sgmunn @slluis