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

More solution API tests #42451

Merged
merged 13 commits into from Mar 26, 2020
Merged

More solution API tests #42451

merged 13 commits into from Mar 26, 2020

Conversation

@tmat
Copy link
Member

tmat commented Mar 15, 2020

Clarifies what inputs are allowed to various solution APIs and adds tests for them.
Fixes adding references to projects outside of the solution - it seems to be allowed by design but assertions were failing in DependencyGraph invariant validations because project ids were missing.
Disallow adding duplicate project references.

Fixes #37124
Fixes #12101
Fixes #34837
Fixes #42406

TODO:

  • Some tests fail because they're passing duplicate metadata references to the ProjectInfo constructors. Should project handle duplicate metadata references?

UPDATE:

  • I'm gonna fix the tests up and keep checking for unique references. RAR deduplicates them, so MSBuildWorkspace should be good.

UPDATE 2:

  • In some cases (when SDK adds portable facades) the references passed to csc task are actually not unique. Need to add filtering of dups to MSBuildWorkspace.
@tmat tmat changed the title More solution tests More solution API tests Mar 15, 2020
@tmat tmat force-pushed the tmat:MoreSolutionTests branch from 78080aa to e638e74 Mar 16, 2020
@tmat tmat marked this pull request as ready for review Mar 16, 2020
@tmat tmat requested review from dotnet/roslyn-compiler as code owners Mar 16, 2020
@tmat

This comment has been minimized.

Copy link
Member Author

tmat commented Mar 16, 2020

@gafter

This comment has been minimized.

Copy link
Member

gafter commented Mar 18, 2020

Finished looking at compiler changes though Iteration 5.

@gafter gafter added this to In Review in Compiler: Gafter Mar 18, 2020
@tmat tmat force-pushed the tmat:MoreSolutionTests branch from 7f92528 to ed6e212 Mar 18, 2020
@gafter
gafter approved these changes Mar 18, 2020
Copy link
Member

gafter left a comment

Compiler changes look fine.

@gafter gafter removed this from In Review in Compiler: Gafter Mar 18, 2020
@jcouv
jcouv approved these changes Mar 18, 2020
Copy link
Member

jcouv left a comment

Compiler change LGTM (iteration 8)

@jcouv jcouv self-assigned this Mar 18, 2020
@tmat tmat closed this Mar 19, 2020
@tmat tmat reopened this Mar 19, 2020
@tmat tmat force-pushed the tmat:MoreSolutionTests branch from 2d0c159 to a06001a Mar 21, 2020
Copy link
Member

jasonmalinowski left a comment

Blocking as there's one copy/paste mistake in the solution tests. Also flagging one question for @sharwell because I thought the dangling project references was handled differently but I could entirely be wrong. I don't want to review that bit until we're on the same page.

I haven't yet reviewed Solution.cs/SolutionState.cs to make sure nothing is being lost oddly in that cleanup, but wanted to get out much of the review so far and to get the open questions out. Have to transition to an urgent PR at the moment...

// add referenced project ids to handle referenced to projects outside of the solution:
projectIds = _projectIds.Union(referencedProjectIds);
Comment on lines 146 to 147

This comment has been minimized.

Copy link
@jasonmalinowski

jasonmalinowski Mar 24, 2020

Member

I believe the intent was the main IDs collection doesn't contain the ones that were floating outside, and those were supposed to be filtered. @sharwell am I mis-remembering?

This comment has been minimized.

Copy link
@tmat

tmat Mar 24, 2020

Author Member

Without this the invariant validation fails, so either the validation is wrong or the _projectIds are wrong.

This comment has been minimized.

Copy link
@sharwell

sharwell Mar 25, 2020

Member

This invariant was added in 53b0c2a and validated here:

foreach (var referencedProject in referencedProjects)
{
Debug.Assert(projectIds.Contains(referencedProject), "Unexpected reference to unknown project.");
}

This comment has been minimized.

Copy link
@sharwell

sharwell Mar 25, 2020

Member

I'd be OK with throwing here if the solution didn't already contain the target of the reference. That can only occur via misuse of a public API.

This comment has been minimized.

Copy link
@jasonmalinowski

jasonmalinowski Mar 25, 2020

Member

That can only occur via misuse of a public API.

The public API explicitly caters to this, namely the distinction between "Project.ProjectReferences" vs. "Project.AllProjectReferences". I don't now if this is a useful feature anymore (at one point MSBuildWorkspace used it I think) and sure wish it didn't exist, but I'd say if we're going to change that let's change that specific thing in our 16.7 feature branch just to get that behavior change in early in a cycle so we can get time to find out what we broke. The rest of this PR is at least changing things we didn't try to support. 😄

Without this the invariant validation fails, so either the validation is wrong or the _projectIds are wrong.

I dug a bit further: so the assumption in the existing implementation (at least at one point) was that the dependency graph isn't told about project references to non-existent projects. Then during project addition we attempt to fix those up:

https://github.com/dotnet/roslyn/blob/master/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.cs#L429-L441

So whatever calls this method the references that go into the ProjectDependencyGraph, since it's fixed up in that AddProject code I linked to above. Note that adding it to the main _projectIds collection can also have other side effects: calls asking for the topologically sorted project IDs would then return them which shouldn't be happening.

This comment has been minimized.

Copy link
@tmat

tmat Mar 25, 2020

Author Member

So, what is the right fix for 16.6? Should I undo this and skip tests that add references to projects that are not in the solution?

@jcouv jcouv removed the Area-Compilers label Mar 24, 2020
Copy link
Member

jasonmalinowski left a comment

Everything looks good other than the project dependency graph change. The specific worry being that this pattern:

solution = solution.AddProjectReference(id, new ProjectReference(idThatDoesNotExist))
var dependencyGraph = solution.GetDependencyGraph();
foreach (var id in dependencyGraph.GetTopologicallySortedProjects())
{
   var project = solution.GetRequiredProject(id);
   /* use project */
}

is busted. And I strongly suspect that pattern exists. Our VisualStudioWorkspace I don't think is relying on project references to things that don't exist -- it sure wasn't intended to -- but I can't promise that. And in any case, somebody else could totally rely on that with their own workspace and an API like Find References which uses the graph like this.

@@ -34,7 +34,7 @@ internal ProjectDependencyGraph WithAdditionalProjectReferences(ProjectId projec
// Note: rather than updating our dependency sets and topologically sorted data, we'll throw that away since incremental update is
// tricky, and those are rarely used. If somebody needs them, it'll be lazily computed.
return new ProjectDependencyGraph(
_projectIds,
_projectIds.Union(referencedProjectIds),

This comment has been minimized.

Copy link
@jasonmalinowski

jasonmalinowski Mar 25, 2020

Member

See comment elsewhere: the existing code was correct (or at least matched the other code); if you're seeing a failure here it means some higher code is failing to filter it out.

@@ -501,9 +567,13 @@ public Solution RemoveProjectReference(ProjectId projectId, ProjectReference pro
/// Create a new solution instance with the project specified updated to contain
/// the specified list of project references.
/// </summary>
public Solution WithProjectReferences(ProjectId projectId, IEnumerable<ProjectReference> projectReferences)
/// <param name="projectId">Id of the project whose references to replace with <paramref name="projectReferences"/>.</param>
/// <param name="projectReferences">New project references. May include duplicate entries.</param>

This comment has been minimized.

Copy link
@jasonmalinowski

jasonmalinowski Mar 25, 2020

Member

Doesn't ToBoxedImmutableArrayWithDistinctNonNullItems throw if there is a duplicate?

This comment has been minimized.

Copy link
@tmat

tmat Mar 25, 2020

Author Member

Ah, yes, I changed it.

CheckContainsProject(projectId);

// avoid enumerating multiple times:
var collection = metadataReferences?.ToCollection();

This comment has been minimized.

Copy link
@jasonmalinowski

jasonmalinowski Mar 25, 2020

Member

Do you want to reassign back to "metadataReferences" to prevent somebody from accidentally touching it again? (Question applies to rest of pattern in this file.)

@tmat tmat force-pushed the tmat:MoreSolutionTests branch from 5e8fc82 to 757b4b9 Mar 26, 2020
Copy link
Member

jasonmalinowski left a comment

Looks good, especially the handling of the project dependency graph insanity. Also nice job cleaning up some of the ickiness around the document file name -> ID map.

@@ -161,10 +161,15 @@ public SolutionState WithNewWorkspace(Workspace workspace, int workspaceVersion)
// [Conditional("DEBUG")]
private void CheckInvariants()
{
Contract.ThrowIfTrue(ProjectIds.Count != _projectIdToProjectStateMap.Count);
Contract.ThrowIfFalse(_projectIdToProjectStateMap.Count == ProjectIds.Count);
Contract.ThrowIfFalse(_projectIdToProjectStateMap.Count == _dependencyGraph.ProjectIds.Count);

This comment has been minimized.

Copy link
@jasonmalinowski

jasonmalinowski Mar 26, 2020

Member

This was the only use for ProjectDependencyGraph.ProjectIds?

@tmat tmat force-pushed the tmat:MoreSolutionTests branch from 757b4b9 to 40f0a74 Mar 26, 2020
@tmat tmat merged commit d64e6cb into dotnet:master Mar 26, 2020
18 checks passed
18 checks passed
WIP Ready for review
Details
license/cla All CLA requirements met.
Details
roslyn-CI Build #20200325.72 succeeded
Details
roslyn-CI (Linux_Test coreclr) Linux_Test coreclr succeeded
Details
roslyn-CI (SourceBuild_Test) SourceBuild_Test succeeded
Details
roslyn-CI (Windows_CoreClr_Unit_Tests debug) Windows_CoreClr_Unit_Tests debug succeeded
Details
roslyn-CI (Windows_CoreClr_Unit_Tests release) Windows_CoreClr_Unit_Tests release succeeded
Details
roslyn-CI (Windows_Correctness_Test) Windows_Correctness_Test succeeded
Details
roslyn-CI (Windows_Desktop_Spanish_Unit_Tests) Windows_Desktop_Spanish_Unit_Tests succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests debug_32) Windows_Desktop_Unit_Tests debug_32 succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests debug_64) Windows_Desktop_Unit_Tests debug_64 succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests release_32) Windows_Desktop_Unit_Tests release_32 succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests release_64) Windows_Desktop_Unit_Tests release_64 succeeded
Details
roslyn-CI (Windows_Determinism_Test) Windows_Determinism_Test succeeded
Details
roslyn-CI (macOS_Test) macOS_Test succeeded
Details
roslyn-integration-CI Build #20200325.71 succeeded
Details
roslyn-integration-CI (VS_Integration debug_async) VS_Integration debug_async succeeded
Details
roslyn-integration-CI (VS_Integration release_async) VS_Integration release_async succeeded
Details
@msftbot msftbot bot added this to the Next milestone Mar 26, 2020
@tmat tmat deleted the tmat:MoreSolutionTests branch Mar 26, 2020
333fred added a commit to 333fred/roslyn that referenced this pull request Mar 26, 2020
* upstream/master: (697 commits)
  Update comment for clarity
  More solution API tests (dotnet#42451)
  Task queue refactoring (dotnet#42610)
  Add string ctor to MemberNotNull/When (dotnet#42712)
  Use the same block structure for code and Metadata As Source
  Add work items
  When generating abstract members, generate them as protected, not internal
  Avoid operations on disposed workspace (dotnet#42768)
  Remove duplicate validation logic
  Speed up regex comment detector pattern.
  Stop walking up once we hit a statement
  Speed up regex comment detector pattern.
  Strengthen the condition in which nodes are determined to be in the same block
  Remove the SolutionPopulator incremental analyzer.
  Update Visualizer version again to fix bug, actually use invariant culture.
  Formatting: force a space after attribute on parameter (dotnet#42466)
  Add test for different case VB
  Update src/EditorFeatures/VisualBasicTest/CodeActions/ReplaceMethodWithProperty/ReplaceMethodWithPropertyTests.vb
  Update src/EditorFeatures/CSharpTest/CodeActions/ReplaceMethodWithProperty/ReplaceMethodWithPropertyTests.cs
  GenerateUniqueName for ReplaceMethodWithProperty
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.