[main] Unlock build failed due to enabling nullable

[v-wuzhai]

No description provided.

[jjonescz]

All the CI failures I can see are like

error NU1301: Unable to load the service index for source https://dotnetfeed.blob.core.windows.net/dotnet-core/index.json.
error NU1301:   Response status code does not indicate success: 409 (Public access is not permitted on this storage account.).

which seems unrelated

[nohwnd]

Is this okay to merge even with the build failures that look unrelated? I am unsure who has to approve here / merge. This is blocking our PRs from building, and I don't want to make more mess by merging this branch into my PRs.

[akoeplinger]

nit: we know this is non-null in this case:

Suggested change

	sw?.ElapsedMilliseconds);
	sw!.ElapsedMilliseconds);

[marcpopMSFT]

FWIW, we got strong guidance to avoid ! where possible by adding null checks. That being said, this seems like the compiler should know that this isn't null here and adding a null check seems incorrect as well. @JeremyKuhne in case he has advise or if this is the exception to that no ! guidance.

[JeremyKuhne]

In this case you can add a Debug.Assert(sw is not null); ahead of it. That will handle the case where someone changes the logic to where this is no longer true. @jaredpar who is the right person to ping about null analysis?

[jaredpar]

It appears this project multi-targetsbetween net472 and other TFMS but runs nullable analysis on all of them. That is the setup I advised against when I went by your desk @marcpopMSFT because it leads to situations like this. Debug.Assert has no impact on nullable analysis when targeting net472 because it doesn't have the new attributes.

Strongly recommend disabling nullable warnings on .NET Framework when you mulit-target with other TFMS.

[kasperk81]

can this wait? 180 pull requests are waiting for someone to press merge here. there is a syntax error in main branch for past 24 hours. the first impulse has to be to fix it or revert the change ASAP

[marcpopMSFT]

I mean, I usually would want to get things right and green before merging as main is not our live branch but apparently enough people are targeting main that I'll go ahead and let it in as is.

[marcpopMSFT]

@jaredpar I tried adding your suggestion of disabling nullable warnings for MT netfx builds but that doesn't work: #45420

I can confirm locally that DisableNullableWarnings is true in the netfix CLI.Utils build but I still get this warning: C:\repos\sdk\src\Cli\Microsoft.DotNet.Cli.Utils\Command.cs(82,21): error CS8602: Dereference of a possibly null reference. [C:\repos\sdk\src\Cli\Microsoft.DotNet.Cli.Utils\Microsoft.DotNet.Cli.Utils.csproj]