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

Cannot redefine/reassign a property in the same file #100

Closed
BinToss opened this issue Mar 10, 2024 · 12 comments
Closed

Cannot redefine/reassign a property in the same file #100

BinToss opened this issue Mar 10, 2024 · 12 comments

Comments

@BinToss
Copy link

BinToss commented Mar 10, 2024

System.ArgumentException: An item with the same key has already been added.
System.InvalidOperationException: Parent project does not have an MSBuild project.

My ZipPublishDir Target defines a Destination property. Then it conditionally appends to the value.

<RepoRoot>$([System.IO.Path]::GetDirectoryName($([MSBuild]::GetPathOfFileAbove('.gitignore', '$(MSBuildProjectDirectory)'))))</RepoRoot>
<Destination>$(RepoRoot)/publish/$(AssemblyName).$(Version)$(PART_RuntimeIdentifier)</Destination>
<Destination Condition="'$(RuntimeIdentifier)' != '' ">$(Destination).$(RuntimeIdentifier)</Destination>

MSBuild reassigns the value to the existing property–or redefines it–as expected.
msbuild-project-tools-server cannot handle reassignments. Instead, it throws System.ArgumentException. Consequently, it cannot load any project importing a file containing this usage.

When adding a key that already exists, msbuild-project-tools-server should

  • A: Assign the new key's value to the existing key. All instances of that property will have the same evaluated value.
  • B: Keep both, but rename duplicates in the order they're discovered. This allows all instances to evaluate to independent values e.g. if the first definition is Destination, then the second definition's key would be Destination>1 and so on.
    • The use of the > character should prevent naming collisions. It cannot be used in a property name—even as &gt;.

error caused by ZipPublishDir

[Error - 5:23:01 AM] Unhandled exception in OnHover.
System.ArgumentException: An item with the same key has already been added. Key: file:///c%3A/Repos/BinToss/GroupBox.Avalonia/node_modules/%40halospv3/hce.shared-config/dotnet/ZipPublishDir.targets
   at bool System.Collections.Generic.Dictionary<TKey, TValue>.TryInsert(TKey key, TValue value, InsertionBehavior behavior)
   at Task<ProjectDocument> MSBuildProjectTools.LanguageServer.Documents.Workspace.GetProjectDocument(Uri documentUri, bool reload)+(Uri _) => { }
   at async Task<ProjectDocument> MSBuildProjectTools.LanguageServer.Documents.Workspace.GetProjectDocument(Uri documentUri, bool reload)
   at async Task<Hover> MSBuildProjectTools.LanguageServer.Handlers.HoverHandler.OnHover(TextDocumentPositionParams parameters, CancellationToken cancellationToken)
   at async Task<Hover> MSBuildProjectTools.LanguageServer.Handlers.HoverHandler.OmniSharp.Extensions.JsonRpc.IRequestHandler<OmniSharp.Extensions.LanguageServer.Protocol.Models.TextDocumentPositionParams,OmniSharp.Extensions.LanguageServer.Protocol.Models.Hover>.Handle(?)

ZipPublishDir

<Project>
  <!-- https://learn.microsoft.com/en-us/visualstudio/msbuild/zipdirectory-task?view=vs-2022 -->
  <Target Name="ZipPublishDir" AfterTargets="Publish">
    <PropertyGroup>
      <RepoRoot>$([System.IO.Path]::GetDirectoryName($([MSBuild]::GetPathOfFileAbove('.gitignore', '$(MSBuildProjectDirectory)'))))</RepoRoot>
      <Destination>$(RepoRoot)/publish/$(AssemblyName).$(Version)$(PART_RuntimeIdentifier)</Destination>
      <Destination Condition="'$(RuntimeIdentifier)' != '' ">$(Destination).$(RuntimeIdentifier)</Destination>
    </PropertyGroup>

    <ZipDirectory
      SourceDirectory="$(PublishDir)"
      DestinationFile="$(Destination).zip"
      Overwrite="true" />
  </Target>
</Project>

error in project importing ZipPublishDir

[Error - 5:36:55 AM] Error loading MSBuild project '"c:\Repos\BinToss\GroupBox.Avalonia\GroupBox.Avalonia.Sample\GroupBox.Avalonia.Sample.csproj"'.
System.InvalidOperationException: Parent project does not have an MSBuild project.
   at bool MSBuildProjectTools.LanguageServer.Documents.SubProjectDocument.TryLoadMSBuildProject()
[Warn  - 5:36:55 AM] Reloaded project file "c:\Repos\BinToss\GroupBox.Avalonia\GroupBox.Avalonia.Sample\GroupBox.Avalonia.Sample.csproj" (XML is valid, but MSBuild project is not).
@tintoy
Copy link
Owner

tintoy commented Mar 10, 2024

Thanks for the detailed report! I’ll look into this tomorrow morning 🙂

@tintoy
Copy link
Owner

tintoy commented Mar 17, 2024

Sorry for the delayed response (production issues at work 😂). I think I can see a solution to this problem, I just need to try it out, and will let you know when there is something to test.

@tintoy
Copy link
Owner

tintoy commented Mar 17, 2024

Part of the problem is that MSBuild has 3 different semantic models (or layers) that can be used to represent various aspects of the project. The language service’s heavy lifting is largely concerned with mapping between the first 2 of these models/layers: Construction (project XML) and Evaluation (evaluated property values and item groups). Part of the issue is that the mapping is not (and cannot always be) one-to-one; the construction project has 2 “Destination” property elements but the evaluation project only has one “Destination” property value.

The original assumption was that we would only track the last declaration of a property in a project and use that one to link it to the actual property in the evaluation project:

public MSBuildProperty(ProjectProperty property, ProjectPropertyElement declaringXml, XSElement propertyElement)

We have done a fair bit of refactoring lately and I suspect a bug has crept in somewhere that is failing to account for multiple declarations.

tintoy added a commit that referenced this issue Mar 24, 2024
…SBuild property defined and then redefined in the same project file

#100
@tintoy
Copy link
Owner

tintoy commented Mar 24, 2024

Ok, I've at least managed to validate that the language model does still correctly support properties defined and then redefined in the same project file. So it must be an upstream component in the language server. Will keep digging :)

@tintoy
Copy link
Owner

tintoy commented Mar 24, 2024

I'm starting to think this may actually be an issue with the targets file being picked up more than once during parsing.

System.ArgumentException: An item with the same key has already been added. Key: file:///c%3A/Repos/BinToss/GroupBox.Avalonia/node_modules/%40halospv3/hce.shared-config/dotnet/ZipPublishDir.targets

@tintoy
Copy link
Owner

tintoy commented Mar 24, 2024

I think 0d40200 may wind up fixing the issue.

tintoy added a commit that referenced this issue Mar 24, 2024
tintoy added a commit that referenced this issue Mar 29, 2024
@BinToss
Copy link
Author

BinToss commented Mar 31, 2024

I still haven't gotten around to building the potential fix and checking if it solves the issue. I'll see if I can do it later today.

@tintoy
Copy link
Owner

tintoy commented Mar 31, 2024

Thanks! I’ve been away for the last couple of days but I’ll try to get a package built tomorrow.

@tintoy
Copy link
Owner

tintoy commented Mar 31, 2024

@BinToss - can you give this package msbuild-project-tools-0.6.3-redefine-property.2.zip a try?

@BinToss
Copy link
Author

BinToss commented Apr 1, 2024

Give the following PropertyGroup of an imported file...

<!-- ZipPublishDir.targets -->
<Project>
	<PropertyGroup>
		<!-- 
			Unused value: false
			Actual value: true
		-->
		<ANewVariable>false</ANewVariable>
		<!-- Value: true -->
		<ANewVariable>true</ANewVariable>
  	</PropertyGroup>

	<Target ...

...the IntelliSense value of ANewVariable is evaluated correctly and without error in consumer projects.

	<!-- Condition Evaluated: true, Value: true -->
	<ANewVariable Condition="$(ANewVariable)">$(ANewVariable)</ANewVariable>

Given this reproducible example, the evaluation issue is not present in that package!

@tintoy
Copy link
Owner

tintoy commented Apr 1, 2024

Great - thanks for verifying! I’ll publish a new version as soon as I’ve addressed some minor PR feedback 🙂

tintoy added a commit that referenced this issue Apr 1, 2024
@tintoy
Copy link
Owner

tintoy commented Apr 1, 2024

I've published v0.6.3 of the extension to the VS gallery (it may take a couple of minutes to show up as an update in VS Code).

Thanks again for contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants