Log NU1105 error for badly specified framework

[Nigusu-Allehu]

Bug

Related: NuGet/Home#12943

Description

Ensure misconfigured projects (specifically projects with badly specified framework) fail with a coded error (NU1105), improving error clarity and debugging

Test project

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <OutputType>Library</OutputType>
    <TargetFrameworks>net.6.0;net.8.0</TargetFrameworks>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Newtonsoft.Json" Version="10.0.3" />
  </ItemGroup>
</Project>

Before

After

PR Checklist

• Meaningful title, helpful description and a linked NuGet/Home issue
• Added tests
• Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc. Document badly specified framework error in Nu1105 Home#14149

[jgonz120]

lgtm, not my area of expertise. Could you provide a screenshot of what the command prompt showed before your change?

[donnie-msft]

I'm assuming the "" will be replaced with some new message that says in a generic way that one or more frameworks were invalid?

[Nigusu-Allehu]

This was on purpose. The logging of the error is done using an ilogger in a previous line.
The exception would look like

error : Invalid restore input. Input files: N:\trash\p7\p7\p7.csproj.

[donnie-msft]

If we're throwing, there's still reason to put a message that makes sense into that exception, regardless of logging that may have happened.

An empty string here can result in an odd message with 2 periods like "Invalid restore input. ."
See:

NuGet.Client/src/NuGet.Core/NuGet.Commands/RestoreCommand/RequestFactory/RestoreSpecException.cs

Line 55 in dd3572a

completeMessage = string.Format(

[Nigusu-Allehu]

I don't think that would happen

NuGet.Client/src/NuGet.Core/NuGet.Commands/Strings.resx

Line 351 in 8101dac

<data name="InvalidRestoreInput" xml:space="preserve">

. The string does not have a period at the end.

If we were to put a message in the exception, it would look something like

NU1105: Invalid framework: ".9"
NU1105: Invalid framework: "unsupp1orted"
NU1105: Invalid framework: "1.222"
error : Invalid restore input. One or more invalid frameworks were detected. Input files: N:\trash\p7\p7\p7.csproj.

If we leave it as an empty message, it will look like

NU1105: Invalid framework: ".9"
NU1105: Invalid framework: "unsupp1orted"
NU1105: Invalid framework: "1.222"
error : Invalid restore input.  Input files: N:\trash\p7\p7\p7.csproj.

Personally, I think adding the message again is redundent.

[donnie-msft]

You're referencing 2 strings here. I'm only talking about InvalidRestoreInput

NuGet.Client/src/NuGet.Core/NuGet.Commands/Strings.Designer.cs

Line 1045 in 13aaaf3

/// Looks up a localized string similar to Invalid restore input. {0}.

When this message is an empty string, I expect "Invalid restore input.__" where __ represents 2 unnecessary spaces.
This does not have "Input files:" in the string.

That would be InvalidRestoreInputWithFiles. Again, I expect the empty string message to create this message: "Invalid restore input.___Input files: {1}." where ___ represents 3 unnecessary spaces and {1} is filled in with filenames.

I'm not following why it's good to omit the message in the exception. I don't think typically we say that logging a message means an exception should be generalized or with empty messages. It may be redundant but is there an issue with such redundancy?

[donnie-msft]

Thx for addressing my earlier comments!

[jeffkl]

I would combine the foreach and only loop through the list once. Its okay to create a list with more items than you end up using. This that case it might make a list with 3 items but only add 2, then after the foreach its just going to throw. But .NET would know that a list with a capacity of 3 really only has 2 items in it.

[donnie-msft]

Also, can you rename f here please.

[Nigusu-Allehu]

I would combine the foreach and only loop through the list once. Its okay to create a list with more items than you end up using. This that case it might make a list with 3 items but only add 2, then after the foreach its just going to throw. But .NET would know that a list with a capacity of 3 really only has 2 items in it.

The second loop is there in hopes of avoiding allocation since we might throw an exception if an invalid framework is found. That way, we don’t allocate a list unless we know we need it. However, given that the list is small, the extra allocation is negligible, and I don’t mind switching to one loop either.

[nkolev92]

Out of curiosity, have you tested how Visual Studio behaves here?
I just wanted to make sure we're not double validating and posting these errors twice in VS.