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

Refresh repo build authoring and sync with microsoft/vs-validation #40

Merged
merged 16 commits into from
Sep 25, 2020
Merged

Refresh repo build authoring and sync with microsoft/vs-validation #40

merged 16 commits into from
Sep 25, 2020

Conversation

scottdorman
Copy link
Contributor

@scottdorman scottdorman commented Sep 19, 2020

Brings the code more in-line with what's in Microsoft.VisualStudio.Validation. This drops support for the two portable libraries as Visual Studio complained that they weren't valid TFMs. This would resolve #38.

…lidation. This drops support for the two portable libraries as Visual Studio complained that they weren't valid TFMs.
@scottdorman
Copy link
Contributor Author

Also, is there a benefit to supporting .NET Standard 1.0 and 1.3 instead of just supporting .NET Standard 1.0? Likewise, the support for .NET 2.0, 4.0, and 4.5. It seems like supporting .NET 2.0, .NET Standard 1.0, and .NET 5 would cover all of the bases.

@scottdorman
Copy link
Contributor Author

Should the simpler methods, like Argument, include a [MethodImpl(MethodImplOptions.AggressiveInlining)] attribute for performance?

@AArnott
Copy link
Owner

AArnott commented Sep 20, 2020

Yes

@scottdorman
Copy link
Contributor Author

scottdorman commented Sep 20, 2020

Would it just be the simpler methods (which I know is a totally subjective thing!) or all of them? I'll add the attribute and submit another commit to the PR.

Hmmm...MethodImplOptions.AggressiveInlining isn't available in .NET 2.0 or 4.0?

@AArnott
Copy link
Owner

AArnott commented Sep 20, 2020

I would put it on any that ms.vs.validation had it on, since we had perf evidence to suggest putting it on those.
We could #if them out on the .NET Framework versions that are missing it. But since .NET 2 and 4.0 are out of support I think, we could drop them too. Same for netstandard1.x.

@scottdorman
Copy link
Contributor Author

scottdorman commented Sep 20, 2020

I'll take a look at what's in ms.vs.validation, although from what I remember, there weren't any methods which had it. There are some which have TargetedPatchingOptOut, which I did copy over. Along those lines, should TargetedPatchingOptOut also be on more (all?) of the methods as well?

So, change the target frameworks to be:

  • .NET 4.5
  • ,NET Standard 1.3
  • .NET Standard 2.0
  • .NET Standard 2.1

…odImpl and TargetedPatchingOptOut attributes.
@AArnott
Copy link
Owner

AArnott commented Sep 21, 2020

Ah, I had AggressiveInlining and TargetedPatchingOptOut confused. I don't think we need AggressiveInlining. The methods are already short and are very likely to be inlined. But TargetedPatchingOptOut is useful for inlining in ngen images across ngen image boundaries.

Copy link
Owner

@AArnott AArnott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for porting all these changes. How would you feel about porting the tests too? I certainly don't want to abuse your willingness to help here, so I'll take your PR either way.
I do have a few smaller changes we should fix up before merging though.

src/Validation/Validation.csproj Outdated Show resolved Hide resolved
src/Validation/Validation.csproj Outdated Show resolved Hide resolved
src/Validation/Validation.csproj Outdated Show resolved Hide resolved
src/Validation/NgenServicingAttributes.cs Outdated Show resolved Hide resolved
src/Validation/InvalidEnumArgumentException.cs Outdated Show resolved Hide resolved
src/Validation/EventHandlerExtensions.cs Outdated Show resolved Hide resolved
src/Validation/Assumes.cs Outdated Show resolved Hide resolved
src/Validation/Report.cs Show resolved Hide resolved
@scottdorman
Copy link
Contributor Author

I can drop the AggressiveInlining, although it probably doesn't hurt to have them there as well.

As for. NET Standard 1.3 support, I don't have a preference either way. Ultimately, it's your choice as the repo owner, but it does make sense to me to drop it. It also makes porting the tests over much easier. I'd actually started to and gave up because too much didn't work without shims or hacks on Net Standard 1.3.

Im happy to help out on this. I was about to port everything into my own validation library, as part of rewriting my project to support. NET 5 and nullable reference types, and then decided it was a better use of my effort to pull it into your library and drop it from mine. (There are a few additional methods I think would be helpful to include from mine before I drop it, but I want to do that as a separate PR. This one is a much bigger impact and should stay focused.

@AArnott
Copy link
Owner

AArnott commented Sep 21, 2020

Great. Let's drop netstandard1.3 then.
I generally oppose adding AggressiveInlining because it is either unnecessary, or second guesses a finely tuned JIT's decisions. If we had evidence that it helped, I'd go for it.

Added serialization constructor and serializable attribute to InternalErrorException.
Pinned language version to C#8.
Fixed the namespace on ExceptionExtensions.
Added a missing #define DEBUG on the Report class.
Added unit tests.
@scottdorman
Copy link
Contributor Author

scottdorman commented Sep 21, 2020

All of the changes have been checked in. For the unit tests, the AssumesTests.InternalErrorException_IsSerializable test was failing with this error when running under netcoreapp3.1:

System.Runtime.Serialization.SerializationException : 
Type 'Microsoft.VisualStudio.TestPlatform.TestHost.DebugAssertException' in 
Assembly 'testhost, Version=15.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' is not marked as serializable.

So that's why it's wrapped in an #if !NETCOREAPP3_1 pragma.

If I remove the ShowAssertDialog method in InternalErrorException, then the test passes on both platforms. That code isn't in the same exception class in ms.vs.validation though, so I'm not sure it's needed.

@scottdorman
Copy link
Contributor Author

Should we leave the two obsolete methods in Assumes or get rid of them? Thinking about it, the fact that these two methods don't pass the showAssert parameter to the implemented methods means that the ShowAssertDialog code in the exception is never called.

…nabled the exception serialization test on .NET Standard 2.1.
…ression and instead override the behavior of the Fail method on a DefaultTraceListener. This allows the tests to run properly on all platforms.
@scottdorman
Copy link
Contributor Author

scottdorman commented Sep 22, 2020

Should the methods all have a [DoesNotReturn] attribute on them (except for the few that do actually return a value, which isn't entirely clear why they do). I know some have a [DoesNotReturnIf(false)] attribute on the bool condition parameter, but that seems wrong to me. These methods will either throw an exception or not, based on condition being true or false, but none of them return. Doesn't the [DoesNotReturnIf] attribute imply that if condition isn't false the method would return a value?

I think I understand this better now. These really have nothing to do with whether or not the method returns a value, just whether or not it completes. (They're somewhat poorly named in that case, in my opinion.)

@AArnott
Copy link
Owner

AArnott commented Sep 25, 2020

Thanks for helping to bring this up to speed. Since this is the first significant change to this repo in quite some time, I took opportunity to apply my Library.Template to it so it works and behaves according to all my other current repos, which adds a bunch of benefits.
I also applied an almost straight file copy from microsoft/vs-validation so that keeping things in sync in the future is easier.
Finally, I removed netstandard2.1 because I saw no point to it. I see you added it explicitly, so if you had a reason for that please enlighten me and we can put it back.

@AArnott AArnott changed the title Master Refresh repo build authoring and sync with microsoft/vs-validation Sep 25, 2020
@AArnott AArnott merged commit 45adbbc into AArnott:master Sep 25, 2020
@scottdorman
Copy link
Contributor Author

You're welcome!

That's a lot of changes for the Library.Template stuff!

The file reformatting (mostly making things alphabetical) was just to keep the order consistent, keep overloaded versions of methods together, and whatnot. Some of the actual code changes were made based on SonarLint warnings for correctness, performance, or maintainability (like single iteration for loops). Personally, it feels like it would be more beneficial to back-port those changes into ms.vs.validation rather than the other way around. Just curious, is there a reason you prefer using explicit types rather than var? Just for readability?

As for netstandard2.1, I added it specifically because ms.vs.validation had it. I guess since that's the first TFM which only targets .NET Core and has the broadest API surface area of all of the .NET Standards, it made sense to include it.

@scottdorman
Copy link
Contributor Author

scottdorman commented Sep 25, 2020

Also, I just synced up my fork, but I'm getting the following error (for both projects) when I try to open the solution:

The project file cannot be opened. Unable to locate the .NET Core SDK. Check that it is installed and that the version specified in global.json (if any) matches the installed version.

Am I missing something on my local machine? If I rename global.json, everything loads properly.

@AArnott
Copy link
Owner

AArnott commented Sep 26, 2020

SonarLint

I've never heard of that. Interesting. Sorry I reverted all those as some of those sound like goodness. Perhaps if we can make the change to vs-validation (first), we can then copy the changes over to this repo.

is there a reason you prefer using explicit types rather than var? Just for readability?

Yes, I favor var when the type is obvious on that line. var x = Something(); is not obvious.
.editorconfig gives us a rule to help get this approximately, so I use that. It does mean that when the type is almost obvious (e.g. var x = Get<IFoo>()) that you still have to spell it out anyway because that's how the compiler enforces it, but it's close enough and better than using var everywhere and not knowing in a code review what the type is.

As for netstandard2.1, I added it specifically because ms.vs.validation had it.

Oh! I wonder why... I don't see any reason for it there either. I think I'll remove it from there too.

it made sense to include it.

netstandard2.0 covers the most runtimes (that are still supported by MS), so if a codebase doesn't require anything beyond that, I prefer to target that. And when the codebase does require netstandard2.1, I'll typically multi-target in order to get netstandard2.0 users something to reference. :)

@AArnott
Copy link
Owner

AArnott commented Sep 26, 2020

I'm getting the following error (for both projects) when I try to open the solution:

Check out the CONTRIBUTING.md file that I also added to the repo. It explains about the SDK dependency and how init.ps1 will install it for you. I recommend using init.ps1 -installlocality machine which requires elevation but puts the SDK in a location where VS will find it no matter where you launch VS from.

@scottdorman
Copy link
Contributor Author

Perhaps if we can make the change to vs-validation (first), we can then copy the changes over to this repo.

I'm ok with that. I can submit a PR, if that's easier. More info on SonarLint is at https://www.sonarlint.org/visualstudio/. There is a VS plugin, as well. It has some really good rules.

I'm good either way with var. I was just curious as to what your preferences are.

LOL. Yea, I thought it was strange that you said you removed netstandard2.0 when it was in ms.vs.validation. :)

Check out the CONTRIBUTING.md file that I also added to the repo.

I'll take a look at it. I didn't even think about looking there, tbh.

@AArnott
Copy link
Owner

AArnott commented Sep 26, 2020

If sonarlint comes as a nuget package I can install in my repos, I'd probably be sold. But analyzers that ship as VSIX's I'm much less of a fan of, since they'll add noise to repos I can't freely change and they aren't enforced by PR builds when others contribute to the repos I do control.

@AArnott
Copy link
Owner

AArnott commented Sep 26, 2020

Oh! I guess it does come as a nuget package: https://www.nuget.org/packages/SonarAnalyzer.CSharp/

Yay. I'll try it out.

@AArnott
Copy link
Owner

AArnott commented Sep 26, 2020

Hmmm... disappointing results. Every single warning it produces in the Validation project is either a duplicate of an existing CA rule or simply a false positive or otherwise suggests a change I wouldn't want to make.

@scottdorman
Copy link
Contributor Author

Oh. That is disappointing. I hadn't used the nuget package, just the vsix extension.

@AArnott
Copy link
Owner

AArnott commented Sep 26, 2020

Do you suppose the results are different between the nuget package and vsix?

@scottdorman
Copy link
Contributor Author

scottdorman commented Sep 26, 2020

It sounds like they are. A good example is this:

When I load the solution and build, I get these warnings from SonarLint on the Requires class:

Warning	S1751	Refactor the containing loop to do more than one iteration.	Validation (net45), Validation (netstandard2.0)	Validation\src\Validation\Requires.cs	196
Warning	S1751	Refactor the containing loop to do more than one iteration.	Validation (net45), Validation (netstandard2.0)	Validation\src\Validation\Requires.cs	228
Warning	S4136	All 'NotNullOrEmpty' method overloads should be adjacent.	Validation (net45), Validation (netstandard2.0)	Validation\src\Validation\Requires.cs	130
Warning	S4144	Update this method so that its implementation is not identical to 'NotNull'.	Validation (net45), Validation (netstandard2.0)	Validation\src\Validation\Requires.cs	113

It sounds like you weren't getting those?

@AArnott
Copy link
Owner

AArnott commented Sep 26, 2020

I was getting those. But they were all useless.

S1751 was pointing at a foreach loop whose only purpose was to see if at least one element was in the collection, so the code was correct.
S4136 I guess I missed. That one is valid.
S4144 False positive. Of the several overloads it might have been referring to (NotNull was not a precise reference) they either did not have identical implementations or they had unique method signatures (one was generic, one wasn't) that was a useful thing.

@scottdorman
Copy link
Contributor Author

scottdorman commented Sep 26, 2020

S4144 False positive.

This is saying that the NotNullAllowStructs method has an identical implementation to NotNull<T>, which it does.

public static T NotNull<T>([ValidatedNotNull, NotNull] T value, string? parameterName)
where T : class // ensures value-types aren't passed to a null checking method
{
if (value is null)
{
throw new ArgumentNullException(parameterName);
}
return value;
}

public static T NotNullAllowStructs<T>([ValidatedNotNull, NotNull] T value, string? parameterName)
{
if (value is null)
{
throw new ArgumentNullException(parameterName);
}
return value;
}

The only difference here is in the type constraint where T : class, which doesn't change the implementation. I do agree that, in this particular case, the warning can be ignored as the important distinction between them is the type constraint.

S1751 was pointing at a foreach loop whose only purpose was to see if at least one element was in the collection, so the code was correct.

Would it not be better, though, to write this a different way and test to see if the enumerator can move forward? Wouldn't that be fewer allocations and branching/conditional testing overall?

bool hasElements = false;
foreach (object value in values)
{
    hasElements = true;
    break;
}

if (!hasElements)
{
    throw new ArgumentException(Format(Strings.Argument_EmptyArray, parameterName), parameterName);
}

would become

using IEnumerator<T> enumerator = values.GetEnumerator();
if (!enumerator.MoveNext())
{
   throw new ArgumentException(Format(Strings.Argument_EmptyArray, parameterName), parameterName);
}

S4136 I guess I missed. That one is valid.

Do you want me to fix that in my other PR?

@AArnott
Copy link
Owner

AArnott commented Sep 27, 2020

I see the two as equivalent. You said "fewer allocations" but what allocation are you saving?

@scottdorman
Copy link
Contributor Author

So, I took a look at both versions in JustDecompile and looked at the IL. For the original version, the IL is

    /// <summary>
    /// Throws an exception if the specified parameter's value is null,
    /// has no elements or has an element with a null value.
    /// </summary>
    /// <typeparam name="T">The type of the elements in the sequence.</typeparam>
    /// <param name="values">The value of the argument.</param>
    /// <param name="parameterName">The name of the parameter to include in any thrown exception.</param>
    /// <exception cref="T:System.ArgumentException">Thrown if the tested condition is false.</exception>
    .method public hidebysig static void NotNullEmptyOrNullElements<class T> (
            class [netstandard]System.Collections.Generic.IEnumerable`1<!!T> values,
            string parameterName
        ) cil managed 
    {
        .custom instance void [netstandard]System.Diagnostics.DebuggerStepThroughAttribute::.ctor() = (
            01 00 00 00
        )
        .param [1]
        .custom instance void Validation.ValidatedNotNullAttribute::.ctor() = (
            01 00 00 00
        )
        .custom instance void System.Diagnostics.CodeAnalysis.NotNullAttribute::.ctor() = (
            01 00 00 00
        )
        .param [2]
        .custom instance void System.Runtime.CompilerServices.NullableAttribute::.ctor(uint8) = (
            01 00 02 00 00
        )
        .locals init (
            [0] bool V_0,
            [1] class [netstandard]System.Collections.Generic.IEnumerator`1<!!T> V_1,
            [2] !!T V_2,
            [3] bool V_3,
            [4] bool V_4
        )

        IL_0000: nop
        IL_0001: ldarg.0
        IL_0002: ldarg.1
        IL_0003: call class [netstandard]System.Collections.Generic.IEnumerable`1<!!0> Validation.Requires::NotNull<class [netstandard]System.Collections.Generic.IEnumerable`1<!!T>>(!!0,  string)
        IL_0008: pop
        IL_0009: ldc.i4.0
        IL_000a: stloc.0
        IL_000b: nop
        IL_000c: ldarg.0
        IL_000d: callvirt instance class [netstandard]System.Collections.Generic.IEnumerator`1<!!0> class [netstandard]System.Collections.Generic.IEnumerable`1<!!T>::GetEnumerator()
        IL_0012: stloc.1
        .try
        {
            IL_0013: br.s IL_0049
            .loop
            {
                IL_0015: ldloc.1
                IL_0016: callvirt instance !!0 class [netstandard]System.Collections.Generic.IEnumerator`1<!!T>::get_Current()
                IL_001b: stloc.2
                IL_001c: nop
                IL_001d: ldc.i4.1
                IL_001e: stloc.0
                IL_001f: ldloc.2
                IL_0020: box !!T
                IL_0025: ldnull
                IL_0026: ceq
                IL_0028: stloc.3
                IL_0029: ldloc.3
                IL_002a: brfalse.s IL_0048

                IL_002c: nop
                IL_002d: call string Validation.Strings::get_Argument_NullElement()
                IL_0032: ldc.i4.1
                IL_0033: newarr [netstandard]System.Object
                IL_0038: dup
                IL_0039: ldc.i4.0
                IL_003a: ldarg.1
                IL_003b: stelem.ref
                IL_003c: call string Validation.Requires::Format(string,  object[])
                IL_0041: ldarg.1
                IL_0042: newobj instance void [netstandard]System.ArgumentException::.ctor(string,  string)
                IL_0047: throw

                IL_0048: nop

                IL_0049: ldloc.1
                IL_004a: callvirt instance bool [netstandard]System.Collections.IEnumerator::MoveNext()
                IL_004f: brtrue.s IL_0015
            }

            IL_0051: leave.s IL_005e
        }
        finally
        {
            IL_0053: ldloc.1
            IL_0054: brfalse.s IL_005d

            IL_0056: ldloc.1
            IL_0057: callvirt instance void [netstandard]System.IDisposable::Dispose()
            IL_005c: nop

            IL_005d: endfinally
        }

        IL_005e: ldloc.0
        IL_005f: ldc.i4.0
        IL_0060: ceq
        IL_0062: stloc.s V_4
        IL_0064: ldloc.s V_4
        IL_0066: brfalse.s IL_0084

        IL_0068: nop
        IL_0069: call string Validation.Strings::get_Argument_EmptyArray()
        IL_006e: ldc.i4.1
        IL_006f: newarr [netstandard]System.Object
        IL_0074: dup
        IL_0075: ldc.i4.0
        IL_0076: ldarg.1
        IL_0077: stelem.ref
        IL_0078: call string Validation.Requires::Format(string,  object[])
        IL_007d: ldarg.1
        IL_007e: newobj instance void [netstandard]System.ArgumentException::.ctor(string,  string)
        IL_0083: throw

        IL_0084: ret
    }

For the version using GetEnumerator, the IL is

    /// <summary>
    /// Throws an exception if the specified parameter's value is null,
    /// has no elements or has an element with a null value.
    /// </summary>
    /// <typeparam name="T">The type of the elements in the sequence.</typeparam>
    /// <param name="values">The value of the argument.</param>
    /// <param name="parameterName">The name of the parameter to include in any thrown exception.</param>
    /// <exception cref="T:System.ArgumentException">Thrown if the tested condition is false.</exception>
    .method public hidebysig static void NotNullEmptyOrNullElements<class T> (
            class [netstandard]System.Collections.Generic.IEnumerable`1<!!T> values,
            string parameterName
        ) cil managed 
    {
        .custom instance void [netstandard]System.Diagnostics.DebuggerStepThroughAttribute::.ctor() = (
            01 00 00 00
        )
        .param [1]
        .custom instance void Validation.ValidatedNotNullAttribute::.ctor() = (
            01 00 00 00
        )
        .custom instance void System.Diagnostics.CodeAnalysis.NotNullAttribute::.ctor() = (
            01 00 00 00
        )
        .param [2]
        .custom instance void System.Runtime.CompilerServices.NullableAttribute::.ctor(uint8) = (
            01 00 02 00 00
        )
        .locals init (
            [0] class [netstandard]System.Collections.Generic.IEnumerator`1<!!T> V_0,
            [1] bool V_1
        )

        IL_0000: nop
        IL_0001: ldarg.0
        IL_0002: ldarg.1
        IL_0003: call class [netstandard]System.Collections.Generic.IEnumerable`1<!!0> Validation.Requires::NotNull<class [netstandard]System.Collections.Generic.IEnumerable`1<!!T>>(!!0,  string)
        IL_0008: pop
        IL_0009: ldarg.0
        IL_000a: callvirt instance class [netstandard]System.Collections.Generic.IEnumerator`1<!!0> class [netstandard]System.Collections.Generic.IEnumerable`1<!!T>::GetEnumerator()
        IL_000f: stloc.0
        .try
        {
            IL_0010: ldloc.0
            IL_0011: callvirt instance bool [netstandard]System.Collections.IEnumerator::MoveNext()
            IL_0016: ldc.i4.0
            IL_0017: ceq
            IL_0019: stloc.1
            IL_001a: ldloc.1
            IL_001b: brfalse.s IL_0039

            IL_001d: nop
            IL_001e: call string Validation.Strings::get_Argument_EmptyArray()
            IL_0023: ldc.i4.1
            IL_0024: newarr [netstandard]System.Object
            IL_0029: dup
            IL_002a: ldc.i4.0
            IL_002b: ldarg.1
            IL_002c: stelem.ref
            IL_002d: call string Validation.Requires::Format(string,  object[])
            IL_0032: ldarg.1
            IL_0033: newobj instance void [netstandard]System.ArgumentException::.ctor(string,  string)
            IL_0038: throw

            IL_0039: leave.s IL_0046
        }
        finally
        {
            IL_003b: ldloc.0
            IL_003c: brfalse.s IL_0045

            IL_003e: ldloc.0
            IL_003f: callvirt instance void [netstandard]System.IDisposable::Dispose()
            IL_0044: nop

            IL_0045: endfinally
        }

        IL_0046: ret
    }

There are fewer entries in .locals init, the body of the .try block is smaller and we lose a box operation and a .loop, after the .try..finally, we lose a bunch of instructions, which appear to be duplicates of what's happening inside the loop (look at IL_002c - IL_0047 and then again at IL_0068 - IL_0083).

@AArnott
Copy link
Owner

AArnott commented Sep 27, 2020

It's not the IL. When we talk about JIT inlining you have to look at the native code disassembly, or look at the ETW events emitted by the JIT.
Looking at the disassembly is relatively easy if you can write it in shortened form. Here on sharplab.io we see that the throw statement does get inlined into the NotNull caller, so we have nothing to worry about. That is, if our simple test case is true to the original. We could copy more of the Requires class into sharplab to see how it works with more confidence.

But the even more sure way is to use perfview so we can measure the actual application we care about and see if the NotNull method call got inlined according to JIT ETW events. But that's significantly more involved, so I don't have time to get into it now.

@scottdorman
Copy link
Contributor Author

Yes, the NotNull is getting inlined (see my comments on #45 as well), but we were looking at the foreach loop. Looking at the JIT Asm on sharplab, I'm still seeing a pretty substantial difference between the two versions (one using the foreach loop and one using IEnumerator<T>.MoveNext() directly).

Granted, I may also be being a bit pedantic on this, so I'm good either way on this one.

@AArnott
Copy link
Owner

AArnott commented Sep 27, 2020

Ah sorry, I got my threads crossed. Ya, interesting stuff about avoiding the loop. Sounds like a change would be an improvement.

@scottdorman
Copy link
Contributor Author

No worries. I'll make the changes and submit a PR. Should I make them here and in vs-validation as well?

@AArnott
Copy link
Owner

AArnott commented Sep 27, 2020

Sure. Thanks.

AArnott added a commit that referenced this pull request May 2, 2023
Update SDK package references
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Freshen this repo up
2 participants