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.

Sign up for GitHub

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

Fix spurious warnings in dependency node #2435

Merged
merged 1 commit into from
Jun 14, 2017
Merged

Fix spurious warnings in dependency node #2435

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ....VS.UnitTests/ProjectSystem/VS/Tree/Dependencies/Models/DiagnosticDependencyModelTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ public void DiagnosticDependencyModelTests_Error()
Assert.False(model.TopLevel);
Assert.Equal(true, model.Visible);
Assert.Equal(null, model.SchemaName);
Assert.Equal(true, model.Resolved);
Assert.Equal(false, model.Resolved);
Assert.Equal(false, model.Implicit);
Assert.Equal(properties, model.Properties);
Assert.Equal(Dependency.DiagnosticsErrorNodePriority, model.Priority);
Expand Down Expand Up @@ -71,7 +71,7 @@ public void DiagnosticDependencyModelTests_Warning()
Assert.False(model.TopLevel);
Assert.Equal(true, model.Visible);
Assert.Equal(null, model.SchemaName);
Assert.Equal(true, model.Resolved);
Assert.Equal(false, model.Resolved);
Assert.Equal(false, model.Implicit);
Assert.Equal(properties, model.Properties);
Assert.Equal(Dependency.DiagnosticsWarningNodePriority, model.Priority);
Expand Down
8 changes: 4 additions & 4 deletions ...ystem.Managed.VS.UnitTests/ProjectSystem/VS/Tree/Dependencies/Snapshot/DependencyTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,8 @@ public void Dependency_Constructor_WhenValidModelProvided_ShouldSetAllProperties
}

[Theory]
[InlineData(@"../../somepath", @"tfm1\xxx\..\..\somepath")]
[InlineData(@"__\somepath..\", @"tfm1\xxx\__\somepath..")]
[InlineData(@"../../somepath", @"tfm1\xxx\__\__\somepath")]
[InlineData(@"__\somepath..\", @"tfm1\xxx\__\somepath__")]
[InlineData(@"somepath", @"tfm1\xxx\somepath")]
public void Dependency_Id_NoSnapsotTargetFramework(string modelId, string expectedId)
{
Expand All @@ -137,8 +137,8 @@ public void Dependency_Id_NoSnapsotTargetFramework(string modelId, string expect
}

[Theory]
[InlineData(@"../../somepath", @"tfm\providerType\..\..\somepath")]
[InlineData(@"__\somepath..\", @"tfm\providerType\__\somepath..")]
[InlineData(@"../../somepath", @"tfm\providerType\__\__\somepath")]
[InlineData(@"__\somepath..\", @"tfm\providerType\__\somepath__")]
[InlineData(@"somepath", @"tfm\providerType\somepath")]
public void Dependency_Id(string modelId, string expectedId)
{
Expand Down
14 changes: 4 additions & 10 deletions ...sts/ProjectSystem/VS/Tree/Dependencies/Snapshot/UnsupportedProjectsSnapshotFilterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ public void UnsupportedProjectsSnapshotFilter_WhenProjectSnapshotFoundAndHasUnre
}

[Fact]
public void UnsupportedProjectsSnapshotFilter_WhenProjectSnapshotNotFound_ShouldMakeUnresolved()
public void UnsupportedProjectsSnapshotFilter_WhenProjectSnapshotNotFound_ShouldDoNothing()
{
// Arrange
var snapshotProvider = IDependenciesSnapshotProviderFactory.Implement(currentSnapshot: null);
Expand All @@ -145,10 +145,7 @@ public void UnsupportedProjectsSnapshotFilter_WhenProjectSnapshotNotFound_Should
topLevel: true,
resolved: true,
flags: DependencyTreeFlags.ProjectNodeFlags.Union(DependencyTreeFlags.ResolvedFlags),
originalItemSpec: @"c:\myproject2\project.csproj",
setPropertiesResolved: false,
setPropertiesSchemaName: ProjectReference.SchemaName,
setPropertiesFlags: DependencyTreeFlags.ProjectNodeFlags.Union(DependencyTreeFlags.UnresolvedFlags));
originalItemSpec: @"c:\myproject2\project.csproj");

var filter = new UnsupportedProjectsSnapshotFilter(aggregateSnapshotProvider, null);

Expand All @@ -166,7 +163,7 @@ public void UnsupportedProjectsSnapshotFilter_WhenProjectSnapshotNotFound_Should
}

[Fact]
public void UnsupportedProjectsSnapshotFilter_WhenProjectSnapshotFoundAndTargetFrameworkNull_ShouldMakeUnresolved()
public void UnsupportedProjectsSnapshotFilter_WhenProjectSnapshotFoundAndTargetFrameworkNull_ShouldDoNothing()
{
// Arrange
var targetFramework = ITargetFrameworkFactory.Implement(moniker: "tfm1");
Expand All @@ -185,10 +182,7 @@ public void UnsupportedProjectsSnapshotFilter_WhenProjectSnapshotFoundAndTargetF
resolved: true,
flags: DependencyTreeFlags.ProjectNodeFlags.Union(DependencyTreeFlags.ResolvedFlags),
originalItemSpec: @"c:\myproject2\project.csproj",
targetFramework: targetFramework,
setPropertiesResolved: false,
setPropertiesSchemaName: ProjectReference.SchemaName,
setPropertiesFlags: DependencyTreeFlags.ProjectNodeFlags.Union(DependencyTreeFlags.UnresolvedFlags));
targetFramework: targetFramework);

var filter = new UnsupportedProjectsSnapshotFilter(aggregateSnapshotProvider, targetFrameworkProvider);

Expand Down
2 changes: 1 addition & 1 deletion ...tSystem.Managed.VS/ProjectSystem/VS/Tree/Dependencies/Models/DiagnosticDependencyModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ public DiagnosticDependencyModel(
ProjectTreeFlags flags,
bool isVisible,
IImmutableDictionary<string, string> properties)
: base(providerType, originalItemSpec, originalItemSpec, flags, resolved:true, isImplicit:false, properties:properties)
: base(providerType, originalItemSpec, originalItemSpec, flags, resolved:false, isImplicit:false, properties:properties)
{
Requires.NotNullOrEmpty(originalItemSpec, nameof(originalItemSpec));
Requires.NotNullOrEmpty(message, nameof(message));
Expand Down
4 changes: 3 additions & 1 deletion ...Studio.ProjectSystem.Managed.VS/ProjectSystem/VS/Tree/Dependencies/Snapshot/Dependency.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,9 @@ private static string GetAlias(IDependency dependency)

private static string Normalize(string id)
{
return id.Replace('/', '\\');
return id
.Replace('/', '\\')
.Replace("..", "__");
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there no API that can do this normalization for us?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a targeted fix to workaround the fact that CPS calls MakeRooted which alters the ID. Long term perhaps we should consider replacing the id with a more opaque hash.

Copy link
Contributor

Choose a reason for hiding this comment

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

This could be problematic. This ID becomes the canonical name of the IVsHierarchy item, and there are places we expect to be able to extract the original file path from the canonical name (for example, analyzer support in Solution Explorer).

Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't be interpreting the moniker - that is only for the project system itself to reason about. You should be using hierarchy properties to retrieve any data you need.

Copy link
Member

Choose a reason for hiding this comment

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

So was this regressed given we merged this in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have not seen a regression in Analyzers. I believe Analyzers use an absolute path so it would not be impacted by this.

Copy link
Member

@davkean davkean Jun 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Okay, I thought about this more, I can see why analyzers did this originally, analyzers look like the following in the project file:

<ItemGroup>
    <Analyzer Include="Analyzer.dll" />
</ItemGroup>

This looks a lot like like source files:

<ItemGroup>
    <Compile Include="Foo.cs"/>
</ItemGroup>

I cansee why we return the analyzer's canonical name as the same as the path of the analyzer.

However, I will state - we should not be expecting or interpreting that outside of the project system. There are other APIs for retrieving a file path given a canonical/mkdocument name, we should be using that. We're theoretically allowed to return whatever we want for a canonical name, for example, that canonical name would be problematic in the following:

<ItemGroup>
    <Analyzer Include="Analyzer.dll" />
    <None Include="Analyzer.dll" /> 
</ItemGroup>

These are going to have the same canonical name even though they are supposed to be unique.

}

public static string GetID(ITargetFramework targetFramework, string providerType, string modelId)
Expand Down
3 changes: 1 addition & 2 deletions .../ProjectSystem/VS/Tree/Dependencies/Snapshot/Filters/UnsupportedProjectsSnapshotFilter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ namespace Microsoft.VisualStudio.ProjectSystem.VS.Tree.Dependencies.Snapshot.Fil
{
/// <summary>
/// Changes resolved top level project dependencies to unresolved if:
/// - dependent project does not have targets supporting given target framework in current project
/// - dependent project has any unresolved dependencies in a snapshot for given target framework
/// This helps to bubble up error status (yellow icon) for project dependencies.
/// </summary>
Expand Down Expand Up @@ -51,7 +50,7 @@ public override IDependency BeforeAdd(
&& !resultDependency.Flags.Contains(DependencyTreeFlags.SharedProjectFlags))
{
var snapshot = GetSnapshot(projectPath, resultDependency, out string dependencyProjectPath);
if (snapshot == null || snapshot.HasUnresolvedDependency)
if (snapshot != null && snapshot.HasUnresolvedDependency)
{
filterAnyChanges = true;
resultDependency = resultDependency.ToUnresolved(ProjectReference.SchemaName);
Expand Down