Evaluation order of ItemGroup Batching suddenly changed in 16.8

[acuntex]

Issue Description

After installing .NET 5 on our build servers, we noticed that the evaluation order of ItemGroup batching changed, making it a breaking change.
The .NET 5 installer replaced MSBuild 16.X with 16.8.
We were able to create a minimalistic repro scenario.

This also happens if a developer updates visual studio 2019 with the installer to the newest version.

Steps to Reproduce

<?xml version="1.0" encoding="utf-8"?>
<Project DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
    <ItemGroup>
        <ArchiveBase Include="CanBeIgnored1">
            <Archive>A</Archive>
        </ArchiveBase>
        <ArchiveBase Include="CanBeIgnored2">
            <Archive>B</Archive>
        </ArchiveBase>
        
        <Installer Include="CanBeIgnored1">
            <Archive>B</Archive>
        </Installer>
    </ItemGroup>

    <Target Name="Build">
        <LogIt
            CreateArchiveName="%(Archive)"
            BaseArchives="@(ArchiveBase->'%(Identity)')"
            Installer="@(Installer->'%(Identity)')" />
    </Target>

    <UsingTask TaskName="LogIt" TaskFactory="CodeTaskFactory" AssemblyFile="$(MSBuildToolsPath)\Microsoft.Build.Tasks.v4.0.dll" >
        <ParameterGroup>
            <CreateArchiveName Required="true" ParameterType="System.String"/>
            <BaseArchives Required="true" ParameterType="Microsoft.Build.Framework.ITaskItem[]"/>
            <Installer Required="true" ParameterType="Microsoft.Build.Framework.ITaskItem[]"/>
        </ParameterGroup>
        <Task>
            <Code Type="Fragment" Language="cs">
            <![CDATA[
                System.Console.WriteLine("CreateArchiveName: " + CreateArchiveName);
                return true;
            ]]>
            </Code>
        </Task>
    </UsingTask>
</Project>

Expected Behavior

CreateArchiveName: A
CreateArchiveName: B

This is also the behavior of MSBuild 16.7 and below.

Actual Behavior

CreateArchiveName: B
CreateArchiveName: A

Analysis

In 16.7 and below the batching order seems to be defined by the order of the attributes of LogIt in the XML. (the first one defines the order)
In 16.8 the batching order seems to be defined by the names of the attributes of LogIt in the XML alphabetically. (the last one defines the order)

Versions & Configurations

Microsoft (R)-Build-Engine, Version 16.8.2+25e4d540b

Attach a binlog

[v-zhiyul]

Hi acuntex, for further investigation, could you provide a minimal project to reproduce this issue? Or detailed steps to manually reproduce this ourselves? Thanks!

[Scordo]

Hi acuntex, for further investigation, could you provide a minimal project to reproduce this issue? Or detailed steps to manually reproduce this ourselves? Thanks!

@v-zhiyul Everything is in the description to reproduce it.

[Scordo]

@v-zhiyul Testet with VS 2017 developer Command prompt and VS 2022 Preview 2 side by side:

[v-zhiyul]

Hi @Scordo thank you for sharing! We can reproduce this issue and contact our engineer in time. Thanks!

[v-zhiyul]

@benvillalobos We verified this issue on VS16.7 and VS16.8. Could you help to take a look?

[benvillalobos]

Oh boy, testing this with a similar repro project and dotnet build I'm seeing:

C:\src\temp\8-30\evaluation\NEW>dotnet build proj\proj.csproj 
Microsoft (R) Build Engine version 16.11.0+0538acc04 for .NET
Copyright (C) Microsoft Corporation. All rights reserved.

  Determining projects to restore...
  All projects are up-to-date for restore.
CreateArchiveName: B
CreateArchiveName: A

AND

C:\src\temp\8-30\evaluation\NEW>dotnet build proj\proj.csproj 
Microsoft (R) Build Engine version 16.11.0+0538acc04 for .NET
Copyright (C) Microsoft Corporation. All rights reserved.

  Determining projects to restore...
  All projects are up-to-date for restore.
CreateArchiveName: A
CreateArchiveName: B

@acuntex do you see similar inconsistent behavior like I am?

[Scordo]

@benvillalobos Acuntex is my fellow and we did not have random results. This construct was used to start an external program multiple times and the order did matter. Thats why we detected a change of logic in 16.8. Because the build suddenly failed. We changed the logic because we couldn't wait for you to fix it. :-) We only tried to give you the smallest solution to reproduce the problem and stopped examine the issue. I've ran the script 100 times in 17.0.0-preview-21329-01+1b7661f36 and 16.10.2+857e5a733 and the result is always: B A

[Scordo]

@benvillalobos I see you're using "dotnet build" instead of "msbuild". It should not make a difference but maybe it does?

[Scordo]

Ok, it's random when using "dotnet build" or "dotnet msbuild":

Code:

<?xml version="1.0" encoding="utf-8"?>
<Project DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
    <ItemGroup>
        <ArchiveBase Include="CanBeIgnored1">
            <Archive>A</Archive>
        </ArchiveBase>
        <ArchiveBase Include="CanBeIgnored2">
            <Archive>B</Archive>
        </ArchiveBase>
        
        <Installer Include="CanBeIgnored1">
            <Archive>B</Archive>
        </Installer>
    </ItemGroup>

    <Target Name="Build">
        <LogIt
            CreateArchiveName="%(Archive)"
            BaseArchives="@(ArchiveBase->'%(Identity)')"
            Installer="@(Installer->'%(Identity)')" />
    </Target>

    <UsingTask TaskName="LogIt" TaskFactory="RoslynCodeTaskFactory" AssemblyFile="$(MSBuildToolsPath)\Microsoft.Build.Tasks.Core.dll" >
        <ParameterGroup>
            <CreateArchiveName Required="true" ParameterType="System.String"/>
            <BaseArchives Required="true" ParameterType="Microsoft.Build.Framework.ITaskItem[]"/>
            <Installer Required="true" ParameterType="Microsoft.Build.Framework.ITaskItem[]"/>
        </ParameterGroup>
        <Task>
            <Code Type="Fragment" Language="cs">
            <![CDATA[
                System.Console.WriteLine("CreateArchiveName: " + CreateArchiveName);
            ]]>
            </Code>
        </Task>
    </UsingTask>
</Project>

[jzabroski]

@benvillalobos @rainersigwald What is the plan to fix this?

Just hit MSB5029! because of this bug!!! Super dangerous bug.