Lazy evaluation optimizations don't respect item transforms #5445

rainersigwald · 2020-06-18T18:24:05Z

Update and Remove don't respect item transforms at evaluation time; they apply to all items instead of just the ones that were selected by the item transform.

This was the case for Update for a long time; Remove was working correctly until #5350.

Steps to reproduce

Project file

<Project>
  <ItemGroup>
    <Foo Include="C:\*" />
    <Foo Include="x" />

    <!-- <Foo Update="@(Foo->WithMetadataValue('Filename', 'x'))" Bar="Baz" /> -->
    <Foo Remove="@(Foo->WithMetadataValue('Filename', 'x'))" />
  </ItemGroup>

  <Target Name="Go">
    <Message Importance="High"
             Text="Foo=@(Foo->'%(Filename) %(Bar)')" />
  </Target>
</Project>

Expected behavior

Only the x item removed/updated

Actual behavior

Go:
  Foo=

Environment data

msbuild /version output: 16.7.0-preview-20309-02+d6862eb21

The text was updated successfully, but these errors were encountered:

rainersigwald · 2020-06-18T18:24:27Z

Thanks for chasing this down @safern!

Ensure that Update and Remove operations done at evaluation time that use item functions pay attention to the item function and don't apply to all items of the same type.

Fixes dotnet#5445 by checking to see if an item function is invoked (the captured expression has subcaptures) before optimizing operations on same-item captures.

Ensure that Update and Remove operations done at evaluation time that use item functions pay attention to the item function and don't apply to all items of the same type.

Fixes dotnet#5445 by checking to see if an item function is invoked (the captured expression has subcaptures) before optimizing operations on same-item captures.

Ensure that Update and Remove operations done at evaluation time that use item functions pay attention to the item function and don't apply to all items of the same type.

Fixes dotnet#5445 by checking to see if an item function is invoked (the captured expression has subcaptures) before optimizing operations on same-item captures.

Ensure that Update and Remove operations done at evaluation time that use item functions pay attention to the item function and don't apply to all items of the same type.

Fixes dotnet#5445 by checking to see if an item function is invoked (the captured expression has subcaptures) before optimizing operations on same-item captures.

…y-eval Fixes #5445 by checking to see if an item function is invoked (the captured expression has subcaptures) before optimizing operations on same-item captures. Added regression tests and improved some asserts that didn't really help me when they fired.

Second part Part 1 of not checking byte parent d86a1e1 author Nathan Mytelka <[email protected]> 1591730709 -0700 committer Nathan Mytelka <[email protected]> 1593461702 -0700 Remove outdated comment Part 2 of unmasking first byte Part 3 Part 4 Part 5 Part 6 Reenabled administrator privilege and cleanup Add test Improve RemoveDependenciesFromEntryIfMissing (dotnet#5392) Fixes dotnet#5180 The fix is a straightforward cache of files that have been detected to exist already, preventing duplicate file system checks on files we already know exist. There will be a future issue and PR addressing ConstructDependencyTable mentioned in a comment on the original issue. Testing on my machine shows an improvement in RemoveDependenciesFromEntryIfMissing of ~850ms down to 30~35ms. Spruce up ObjectModelHelpers assertions These fired while I was writing a new test but didn't have much useful information. Keep AssertItems from throwing an ArgumentOutOfRangeException on mismatched lengths, and give a clue or two about mismatched lengths in AssertItemHasMetadata. Regression tests for dotnet#5445 Ensure that Update and Remove operations done at evaluation time that use item functions pay attention to the item function and don't apply to all items of the same type. Respect item functions in lazy Update/Remove Fixes dotnet#5445 by checking to see if an item function is invoked (the captured expression has subcaptures) before optimizing operations on same-item captures. Rename short-circuit-lazy-item-update check method The question this method answers is 'can I just remove/update every item in the group, or do I need to expand the value to match against existing items?' Renamed it for a bit more clarity there. Log CurrentUICulture in binlog (dotnet#5426) This will be useful to be able to open localized binlogs. If we know the culture of the log we can fetch the right resources from the MSBuild .dlls. Properly parse version Part 7

* First step Second part Part 1 of not checking byte parent d86a1e1 author Nathan Mytelka <[email protected]> 1591730709 -0700 committer Nathan Mytelka <[email protected]> 1593461702 -0700 Remove outdated comment Part 2 of unmasking first byte Part 3 Part 4 Part 5 Part 6 Reenabled administrator privilege and cleanup Add test Improve RemoveDependenciesFromEntryIfMissing (#5392) Fixes #5180 The fix is a straightforward cache of files that have been detected to exist already, preventing duplicate file system checks on files we already know exist. There will be a future issue and PR addressing ConstructDependencyTable mentioned in a comment on the original issue. Testing on my machine shows an improvement in RemoveDependenciesFromEntryIfMissing of ~850ms down to 30~35ms. Spruce up ObjectModelHelpers assertions These fired while I was writing a new test but didn't have much useful information. Keep AssertItems from throwing an ArgumentOutOfRangeException on mismatched lengths, and give a clue or two about mismatched lengths in AssertItemHasMetadata. Regression tests for #5445 Ensure that Update and Remove operations done at evaluation time that use item functions pay attention to the item function and don't apply to all items of the same type. Respect item functions in lazy Update/Remove Fixes #5445 by checking to see if an item function is invoked (the captured expression has subcaptures) before optimizing operations on same-item captures. Rename short-circuit-lazy-item-update check method The question this method answers is 'can I just remove/update every item in the group, or do I need to expand the value to match against existing items?' Renamed it for a bit more clarity there. Log CurrentUICulture in binlog (#5426) This will be useful to be able to open localized binlogs. If we know the culture of the log we can fetch the right resources from the MSBuild .dlls. Properly parse version Part 7 * Moved user check * Moved Handshake * Fixed build * Refactoring * Move fire byte calculation into loop * Save before committing 😃 * Catch uncaught exception * PR feedback * Correct off-by-one error

* First step Second part Part 1 of not checking byte parent d86a1e1 author Nathan Mytelka <[email protected]> 1591730709 -0700 committer Nathan Mytelka <[email protected]> 1593461702 -0700 Remove outdated comment Part 2 of unmasking first byte Part 3 Part 4 Part 5 Part 6 Reenabled administrator privilege and cleanup Add test Improve RemoveDependenciesFromEntryIfMissing (#5392) Fixes #5180 The fix is a straightforward cache of files that have been detected to exist already, preventing duplicate file system checks on files we already know exist. There will be a future issue and PR addressing ConstructDependencyTable mentioned in a comment on the original issue. Testing on my machine shows an improvement in RemoveDependenciesFromEntryIfMissing of ~850ms down to 30~35ms. Spruce up ObjectModelHelpers assertions These fired while I was writing a new test but didn't have much useful information. Keep AssertItems from throwing an ArgumentOutOfRangeException on mismatched lengths, and give a clue or two about mismatched lengths in AssertItemHasMetadata. Regression tests for #5445 Ensure that Update and Remove operations done at evaluation time that use item functions pay attention to the item function and don't apply to all items of the same type. Respect item functions in lazy Update/Remove Fixes #5445 by checking to see if an item function is invoked (the captured expression has subcaptures) before optimizing operations on same-item captures. Rename short-circuit-lazy-item-update check method The question this method answers is 'can I just remove/update every item in the group, or do I need to expand the value to match against existing items?' Renamed it for a bit more clarity there. Log CurrentUICulture in binlog (#5426) This will be useful to be able to open localized binlogs. If we know the culture of the log we can fetch the right resources from the MSBuild .dlls. Properly parse version Part 7 * Moved user check * Moved Handshake * Fixed build * Refactoring * Move fire byte calculation into loop * Save before committing 😃 * Catch uncaught exception * PR feedback * Correct off-by-one error * Make handshake version explicit * PR comments

dotnet#291 - introduced a change to skip some 3rd party assemblies for the signing step, but this broke because of a msbuild issue: dotnet/msbuild#5445 . - Essentially, it ended up removing *all* the items to sign.

@premun

#291 - introduced a change to skip some 3rd party assemblies for the signing step, but this broke because of a msbuild issue: dotnet/msbuild#5445 . - Essentially, it ended up removing *all* the items to sign. * Explicitly list the 3rd part libs to sign - Based on @premun's comment (#293 (comment)) - Also remove `AllowEmptySignList=true`, so that we can catch situations where the `ItemsToSign` becomes empty for some reason

rainersigwald added this to the MSBuild 16.7 Preview 4 milestone Jun 18, 2020

rainersigwald self-assigned this Jun 18, 2020

safern mentioned this issue Jun 18, 2020

Fix signing by moving ItemsToSign evaluation to be inside a target dotnet/runtime#38104

Merged

rainersigwald mentioned this issue Jun 18, 2020

Respect transforms in lazy eval #5447

Merged

safern mentioned this issue Jun 22, 2020

Visual Studio issues with building simple console app dotnet/runtime#38105

Closed

rainersigwald closed this as completed in #5447 Jun 24, 2020

radical mentioned this issue Aug 10, 2020

Fix signing that broke with an earlier commit dotnet/xharness#293

Merged

AR-May added the triaged label Feb 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lazy evaluation optimizations don't respect item transforms #5445

Lazy evaluation optimizations don't respect item transforms #5445

rainersigwald commented Jun 18, 2020

rainersigwald commented Jun 18, 2020

Lazy evaluation optimizations don't respect item transforms #5445

Lazy evaluation optimizations don't respect item transforms #5445

Comments

rainersigwald commented Jun 18, 2020

Steps to reproduce

Expected behavior

Actual behavior

Environment data

rainersigwald commented Jun 18, 2020