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

Report bad tightening/loosening of attributes in OHI #41336

Merged
merged 13 commits into from Feb 12, 2020
Merged

Conversation

@jcouv
Copy link
Member

jcouv commented Jan 31, 2020

Implements the last part of #36039 (new enforcement of nullable attributes).

In terms of design:

  • we're validating that parameter values can be assigned between overridden and overriding method (or implemented/implementing) accounting for nullable attributes and ref kind
  • we're validating that [Maybe/NotNull] attributes on by-value/in parameters are matching
@@ -108,3 +108,16 @@ Similarly, a value from a `[DisallowNull] ref string? p` parameter will be assum
On the other hand, we'll warn for returning a `null` from a method declared with `[NotNull] string?` return type, and we'll treat the value from a `[AllowNull] ref string p` parameter as maybe-null.
Conditional attributes are treated leniently. For instance, no warning will be produced for assigning a maybe-null value to an `[MaybeNullWhen(...)] out string p` parameter.

19. https://github.com/dotnet/roslyn/issues/36039 In *Visual Studio 2019 version 16.5* and onwards, the compiler did not check the usage of nullable flow annotation attributes, such as `[MaybeNull]` or `[NotNull]`, in overrides or implementations. In *Visual Studio 2019 version 16.5*, those usages are checked to respect null discipline. For example:

This comment has been minimized.

Copy link
@sharwell

sharwell Jan 31, 2020

Member
Suggested change
19. https://github.com/dotnet/roslyn/issues/36039 In *Visual Studio 2019 version 16.5* and onwards, the compiler did not check the usage of nullable flow annotation attributes, such as `[MaybeNull]` or `[NotNull]`, in overrides or implementations. In *Visual Studio 2019 version 16.5*, those usages are checked to respect null discipline. For example:
19. https://github.com/dotnet/roslyn/issues/36039 Prior to *Visual Studio 2019 version 16.5*, the compiler did not check the usage of nullable flow annotation attributes, such as `[MaybeNull]` or `[NotNull]`, in overrides or implementations. In *Visual Studio 2019 version 16.5* and onwards, those usages are checked to respect null discipline. For example:
``` #Resolved
The use of any member returning `[MaybeNull]T` for an unconstrained type parameter `T` produces a warning, just like `default(T)` (see below). For instance, `listT.FirstOrDefault();` would produce a warning.

Enforcing nullability attributes within method bodies:
- An input parameter marked with [AllowNull] is initialized with a maybe-null (or maybe-default) state.

This comment has been minimized.

Copy link
@sharwell

sharwell Jan 31, 2020

Member
Suggested change
- An input parameter marked with [AllowNull] is initialized with a maybe-null (or maybe-default) state.
- An input parameter marked with `[AllowNull]` is initialized with a maybe-null (or maybe-default) state.
``` #Resolved

Enforcing nullability attributes within method bodies:
- An input parameter marked with [AllowNull] is initialized with a maybe-null (or maybe-default) state.
- An input parameter marked with [DisallowNull] is initialized with a not-null state.

This comment has been minimized.

Copy link
@sharwell

sharwell Jan 31, 2020

Member
Suggested change
- An input parameter marked with [DisallowNull] is initialized with a not-null state.
- An input parameter marked with `[DisallowNull]` is initialized with a not-null state.
``` #Resolved
Enforcing nullability attributes within method bodies:
- An input parameter marked with [AllowNull] is initialized with a maybe-null (or maybe-default) state.
- An input parameter marked with [DisallowNull] is initialized with a not-null state.
- A parameter marked with [MaybeNull] or [MaybeNullWhen] can be assigned a maybe-null value, without warning. Same for return values. Same for a nullable parameter marked with [NotNullWhen] (the attribute is ignored).

This comment has been minimized.

Copy link
@sharwell

sharwell Jan 31, 2020

Member
Suggested change
- A parameter marked with [MaybeNull] or [MaybeNullWhen] can be assigned a maybe-null value, without warning. Same for return values. Same for a nullable parameter marked with [NotNullWhen] (the attribute is ignored).
- A parameter marked with `[MaybeNull]` or `[MaybeNullWhen]` can be assigned a maybe-null value, without warning. Same for return values. Same for a nullable parameter marked with `[NotNullWhen]` (the attribute is ignored).
``` #Resolved
- An input parameter marked with [AllowNull] is initialized with a maybe-null (or maybe-default) state.
- An input parameter marked with [DisallowNull] is initialized with a not-null state.
- A parameter marked with [MaybeNull] or [MaybeNullWhen] can be assigned a maybe-null value, without warning. Same for return values. Same for a nullable parameter marked with [NotNullWhen] (the attribute is ignored).
- A parameter marked with [NotNull] will produce a warning when assigned a maybe-null value. Same for return values.

This comment has been minimized.

Copy link
@sharwell

sharwell Jan 31, 2020

Member

This only applies to out and ref parameters right?

Suggested change
- A parameter marked with [NotNull] will produce a warning when assigned a maybe-null value. Same for return values.
- A by-reference parameter marked with `[NotNull]` will produce a warning when assigned a maybe-null value. Same for return values.
``` #Resolved

This comment has been minimized.

Copy link
@jcouv

jcouv Feb 11, 2020

Author Member

@sharwell, no this applies to by-value parameters as well. I'll update test UnconditionalAttributesAffectMethodBodies_ReferenceType to show that behavior.


In reply to: 373548582 [](ancestors = 373548582)

- An input parameter marked with [DisallowNull] is initialized with a not-null state.
- A parameter marked with [MaybeNull] or [MaybeNullWhen] can be assigned a maybe-null value, without warning. Same for return values. Same for a nullable parameter marked with [NotNullWhen] (the attribute is ignored).
- A parameter marked with [NotNull] will produce a warning when assigned a maybe-null value. Same for return values.
- The state of a parameter marked with [Maybe/NotNullWhen] is checked upon exiting the method instead.

This comment has been minimized.

Copy link
@sharwell

sharwell Jan 31, 2020

Member
Suggested change
- The state of a parameter marked with [Maybe/NotNullWhen] is checked upon exiting the method instead.
- The state of a parameter marked with `[MaybeNullWhen]`/`[NotNullWhen]` is checked upon exiting the method instead.
``` #Resolved
- The state of a parameter marked with [Maybe/NotNullWhen] is checked upon exiting the method instead.
- A method marked with [DoesNotReturn] will produce a warning if it returns or exits normally.

Note: we don't validate the internal consistency of auto-properties, so it is possible to misused attributes on auto-props as on fields. For example: `[AllowNull, NotNull] public TOpen P { get; set; }`.

This comment has been minimized.

Copy link
@sharwell

sharwell Jan 31, 2020

Member

💡 Consider breaking out a section specifically to point out things that are not checked #WontFix


Enforcing nullability attributes when overriding and implementing:
In addition to checking the types in overrides/implementations are compatible with overridden/implemented members, we also check that the nullability attributes are compatible.
- For input parameters (by-value and `in`), we check that the worst allowed value by the overridden/implemented parameter can be assigned to the overriding/implementing parameter.

This comment has been minimized.

Copy link
@sharwell

sharwell Jan 31, 2020

Member
Suggested change
- For input parameters (by-value and `in`), we check that the worst allowed value by the overridden/implemented parameter can be assigned to the overriding/implementing parameter.
- For input parameters (by-value and `in`), we check that the widest allowed value by the overridden/implemented parameter can be assigned to the overriding/implementing parameter.
``` #Resolved
Enforcing nullability attributes when overriding and implementing:
In addition to checking the types in overrides/implementations are compatible with overridden/implemented members, we also check that the nullability attributes are compatible.
- For input parameters (by-value and `in`), we check that the worst allowed value by the overridden/implemented parameter can be assigned to the overriding/implementing parameter.
- For output parameters (`out` and return values), we check that the worst produced value by the overriding/implementing parameter can be assigned to the overridden/implemented parameter.

This comment has been minimized.

Copy link
@sharwell

sharwell Jan 31, 2020

Member
Suggested change
- For output parameters (`out` and return values), we check that the worst produced value by the overriding/implementing parameter can be assigned to the overridden/implemented parameter.
- For output parameters (`out` and return values), we check that the widest produced value by the overriding/implementing parameter can be assigned to the overridden/implemented parameter.
``` #Resolved
- For input parameters (by-value and `in`), we check that the worst allowed value by the overridden/implemented parameter can be assigned to the overriding/implementing parameter.
- For output parameters (`out` and return values), we check that the worst produced value by the overriding/implementing parameter can be assigned to the overridden/implemented parameter.
- For `ref` parameters and return values, we check both.
- We check that the post-condition contract `[Not/MaybeNull]` on input parameters is enforced by overriding/implementing members.

This comment has been minimized.

Copy link
@sharwell

sharwell Jan 31, 2020

Member
Suggested change
- We check that the post-condition contract `[Not/MaybeNull]` on input parameters is enforced by overriding/implementing members.
- We check that the post-condition contract `[NotNull]`/`[MaybeNull]` on input parameters is present and enforced by overriding/implementing members.
``` #Resolved
- For output parameters (`out` and return values), we check that the worst produced value by the overriding/implementing parameter can be assigned to the overridden/implemented parameter.
- For `ref` parameters and return values, we check both.
- We check that the post-condition contract `[Not/MaybeNull]` on input parameters is enforced by overriding/implementing members.
- We check that overrides/implementations have the `[DoesNotReturn]` attribute if the overridden/implemented member has it.

This comment has been minimized.

Copy link
@sharwell

sharwell Jan 31, 2020

Member

Do we allow an implementation to use [DoesNotReturn] if the original definition used [DoesNotReturnIf]? #Resolved

This comment has been minimized.

Copy link
@jcouv

jcouv Feb 3, 2020

Author Member

I'll add a test, but I think we allow it and that behavior is correct.
The implementation abides by the overridden/implemented method's contract.


In reply to: 373550920 [](ancestors = 373550920)

This comment has been minimized.

Copy link
@jcouv

jcouv Feb 5, 2020

Author Member

It turns out the problem doesn't even present itself. [DoesNotReturn] is applied to methods, but [DoesNotReturnIf(...)] is applied to parameters. They don't cross paths. #Resolved

@jcouv jcouv force-pushed the jcouv:nullable-ohi branch from be3b978 to f6c7427 Feb 3, 2020
@jcouv jcouv force-pushed the jcouv:nullable-ohi branch 2 times, most recently from 84385ec to d2194ab Feb 3, 2020
@jcouv jcouv marked this pull request as ready for review Feb 3, 2020
@jcouv jcouv requested a review from dotnet/roslyn-compiler as a code owner Feb 3, 2020
@jcouv jcouv force-pushed the jcouv:nullable-ohi branch from d2194ab to 6eb6655 Feb 3, 2020
@jcouv jcouv force-pushed the jcouv:nullable-ohi branch from cbe1cd9 to 3b9d591 Feb 4, 2020
- An input parameter marked with `[DisallowNull]` is initialized with a not-null state.
- A parameter marked with `[MaybeNull]` or `[MaybeNullWhen]` can be assigned a maybe-null value, without warning. Same for return values. Same for a nullable parameter marked with `[NotNullWhen]` (the attribute is ignored).
- A parameter marked with `[NotNull]` will produce a warning when assigned a maybe-null value. Same for return values.
- The state of a parameter marked with `[MaybeNullWhen]`\`[NotNullWhen]` is checked upon exiting the method instead.

This comment has been minimized.

Copy link
@cston

cston Feb 4, 2020

Member

\ [](start = 56, length = 1)

"/" or "or" #Closed

bool overriddenHasNotNull = (overriddenAnnotations & FlowAnalysisAnnotations.NotNull) == FlowAnalysisAnnotations.NotNull;
if (overriddenHasNotNull && !overridingHasNotNull && !forRef)
{
// Overriding doesn't conform to contract of overridden (ie. promise not to return if parameter is null)

This comment has been minimized.

Copy link
@cston

cston Feb 4, 2020

Member

promise not to return if parameter is null [](start = 81, length = 42)

What does "promise not to return" mean for parameters? #Closed

This comment has been minimized.

Copy link
@jcouv

jcouv Feb 4, 2020

Author Member

void AssertNotNull<T>([NotNull] T t) would be an example. The method promises not to return if the parameter is null. #Closed

{
// Can't assign value from overriding to overridden in true case
return false;
}

This comment has been minimized.

Copy link
@cston

cston Feb 4, 2020

Member

Consider extracting a helper method, with sense parameter, for use here and false case below. #Closed

}
:
(Action<DiagnosticBag, MethodSymbol, MethodSymbol, ParameterSymbol, Location>)null,
checkReturnType ? reportBadReturn : (ReportMismatchinReturnType<Location>)null,

This comment has been minimized.

Copy link
@cston

cston Feb 4, 2020

Member

(ReportMismatchinReturnType) [](start = 85, length = 38)

Are the casts needed here or for checkParameters? #Closed

This comment has been minimized.

Copy link
@jcouv

jcouv Feb 5, 2020

Author Member

Yes, the casts are necessary. #Resolved

}
:
(Action<DiagnosticBag, MethodSymbol, MethodSymbol, ParameterSymbol, Location>)null,
checkReturnType ? reportBadReturn : (ReportMismatchinReturnType<Location>)null,

This comment has been minimized.

Copy link
@cston

cston Feb 4, 2020

Member

reportBadReturn [](start = 67, length = 15)

The local functions will result in delegate instances. Consider inlining the local functions instead. #Closed

reportMismatchInParameterType(diagnostics, overriddenMethod, overridingMethod, overridingParameters[i], extraArgument);
reportMismatchInParameterType(diagnostics, overriddenMethod, overridingMethod, parameter, false, extraArgument);
}
else if (reportMismatchInParameterType != null &&

This comment has been minimized.

Copy link
@cston

cston Feb 4, 2020

Member

reportMismatchInParameterType != null [](start = 25, length = 37)

Already checked before the for loop. #Closed

@@ -1104,14 +1126,25 @@ static bool isOrContainsErrorType(TypeSymbol typeSymbol)
for (int i = 0; i < overriddenMethod.ParameterCount; i++)
{
var overriddenParameterType = overriddenParameters[i].TypeWithAnnotations;

This comment has been minimized.

Copy link
@cston

cston Feb 4, 2020

Member

overriddenParameters[i] [](start = 46, length = 23)

Consider extracting a local. #Closed

{
diagnostics.Add(ErrorCode.WRN_DoesNotReturnMismatch, overridingMethod.Locations[0], new FormattedSymbol(overridingMethod, SymbolDisplayFormat.MinimallyQualifiedFormat));
}

var conversions = compilation.Conversions.WithNullability(true);
if (reportMismatchInReturnType != null &&

This comment has been minimized.

Copy link
@cston

cston Feb 4, 2020

Member

reportMismatchInReturnType != null [](start = 16, length = 34)

Please consider checking this condition once, around this case and the one below. #Closed

if (overriddenHasNotNull && !overridingHasNotNull && !forRef)
{
// Overriding doesn't conform to contract of overridden (ie. promise not to return if parameter is null)
return false;

This comment has been minimized.

Copy link
@cston

cston Feb 4, 2020

Member

false [](start = 27, length = 5)

Do we get here with substituted types?

abstract class A<T>
{
    [return: NotNull] abstract T F();
}
class B : A<string?>
{
    override string F() => string.Empty;
}

Or cases where override differs by attributes but not by actual nullability?

abstract class A
{
    [return: MaybeNull] abstract string F();
}
class B : A<string?>
{
    override string? F() => null;
}
``` #Closed

This comment has been minimized.

Copy link
@jcouv

jcouv Feb 5, 2020

Author Member

We only get here with input parameters. [NotNull] takes a special meaning on inputs, namely "the method will not return if the input is null". This contract can only be represented with the [NotNull] attribute and cannot be represented with types. So we need the attribute regardless of substitution.

I'll double-check the behavior for ref scenarios. The !forRef check might be wrong. The behavior of [NotNull] ref parameter is correct. There [NotNull] strictly has it's post-condition meaning. #Resolved

@cston

This comment has been minimized.

Copy link
Member

cston commented Feb 4, 2020

    public void AllowNull_OHI_OnInterface()

Consider dropping the "_OHI" qualifier from tests if it's not needed. (Hiding is not included and the specific overriding or implementing is usually clear from the test name.) #Closed


Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:20882 in 3b9d591. [](commit_id = 3b9d591, deletion_comment = False)

@cston

This comment has been minimized.

Copy link
Member

cston commented Feb 4, 2020

        // Note: warnings on F4 aren't great, but are probably deserved

There are no warnings on F4. #Closed


Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:29419 in 3b9d591. [](commit_id = 3b9d591, deletion_comment = False)

@RikkiGibson

This comment has been minimized.

Copy link
Contributor

RikkiGibson commented Feb 11, 2020

public virtual void F1<T>([AllowNull]T  t1, [AllowNull] out T  t2, [AllowNull] ref T  t3, [AllowNull] in T  t4)                  => throw null!;

Is use of nullability attributes outside of a nullable context something we intend to support? Or are we just recording the existing behavior? #Resolved


Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:29473 in 4ab452c. [](commit_id = 4ab452c, deletion_comment = False)

@RikkiGibson

This comment has been minimized.

Copy link
Contributor

RikkiGibson commented Feb 11, 2020

[MaybeNull] public override string P2 { set { throw null!; } } // 0

I don't see this diagnostic in VerifyDiagnostics #Resolved


Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:21475 in 4ab452c. [](commit_id = 4ab452c, deletion_comment = False)

overridingType.ToTypeWithState(),
makeUnconditionalAnnotation(overridingAnnotations, sense));

var destAnnotationsWhenTrue = ToInwardAnnotations(makeUnconditionalAnnotation(overriddenAnnotations, sense));

This comment has been minimized.

Copy link
@RikkiGibson

RikkiGibson Feb 11, 2020

Contributor

ToInwardAnnotations [](start = 46, length = 19)

What does ToInwardAnnotations mean? Does it refer to annotations that affect input, e.g. AllowNull? Maybe it would be better to use the term inbound or input as it has had other uses throughout the walker and we can avoid using multiple terms to refer to the same specific thing. #Resolved

This comment has been minimized.

Copy link
@jcouv

jcouv Feb 11, 2020

Author Member

If you have a void M([AllowNull] string x) method, inside the body NullableWalker needs to see the inwards annotation of x, ie. [MaybeNull].
That way the initial state of x will be maybe-null (since we allow null from the caller).
"Inward" is a different concept, to refer to this reversal. #Resolved

if (refKind == RefKind.Ref)
{
// ref variables are invariant
return AreParameterAnnotationsCompatible(RefKind.None, overriddenType, overriddenAnnotations, overridingType, overridingAnnotations, forRef: true) &&

This comment has been minimized.

Copy link
@RikkiGibson

RikkiGibson Feb 11, 2020

Contributor

Why do we pass in RefKind.None and also forRef: true? What distinguishes the meaning of those arguments? #Resolved

This comment has been minimized.

Copy link
@jcouv

jcouv Feb 11, 2020

Author Member

On a RefKind.None parameter, [NotNull] takes a special meaning. We don't want that meaning to apply to a ref argument when it is analyzed as RefKind.None + RefKind.Out. #Resolved

@@ -1157,6 +1160,121 @@ static bool isMaybeDefaultValue(TypeWithState valueType)
}
}

internal static bool AreParameterAnnotationsCompatible(

This comment has been minimized.

Copy link
@RikkiGibson

RikkiGibson Feb 11, 2020

Contributor

Perhaps this should be named CheckOverridingParameterAnnotations? Since we tend to "blame" the overriding method for not conforming to the requirements of the overridden method? Also wondering if it might make sense to declare closer to its use site in SourceMemberContainerSymbol_ImplementationChecks. #WontFix

This comment has been minimized.

Copy link
@jcouv

jcouv Feb 11, 2020

Author Member

I initially has this method in SourceMemberContainerSymbol_ImplementationChecks, but that required to expose multiple private methods of NulllableWalker as internal. So I figured this was a more natural place.
I'll adjust the name #Resolved

This comment has been minimized.

Copy link
@jcouv

jcouv Feb 11, 2020

Author Member

I ended up keeping the current name because this method doesn't report any diagnostics. The caller is assigning blame to the overriding/implementing parameter ;-)

Also, "CheckXYZ" doesn't convey that it returns a boolean. So I prefer an IsXYZ or AreXYZ convention.


In reply to: 377404557 [](ancestors = 377404557)

@RikkiGibson

This comment has been minimized.

Copy link
Contributor

RikkiGibson commented Feb 11, 2020

Done with review pass (iteration 7) #Resolved

@jcouv

This comment has been minimized.

Copy link
Member Author

jcouv commented Feb 11, 2020

Created PR dotnet/runtime#32090 for impact of this change on runtime repo. Tagging @jaredpar as FYI #Resolved

@jcouv

This comment has been minimized.

Copy link
Member Author

jcouv commented Feb 11, 2020

Is use of nullability attributes outside of a nullable context something we intend to support? Or are we just recording the existing behavior?

We are just recording the existing behavior. #Resolved

@jcouv

This comment has been minimized.

Copy link
Member Author

jcouv commented Feb 11, 2020

Does [NotNullWhen] just not do anything by design for None/In parameters? I would have expected a similar behavior to [NotNull] where we make a claim that the argument is not-null when the method returns a certain value.

That would make sense, but I'm not sure I have a compelling example.
Filed #41569 #Resolved

@jcouv

This comment has been minimized.

Copy link
Member Author

jcouv commented Feb 11, 2020

Looking at T t1 specifically: does it make sense for users to write a method which "introduces doubt" about the null state in the way that x != null or x?.y checks do?

A void M([MaybeNull] ...) method does that. In M(expr) we will learn that the expression may be null, just as we do in expr == null.

using System.Diagnostics.CodeAnalysis;
#nullable enable

public class C 
{
    public void M([MaybeNull] string x) 
    {
    }
    
    public void M2(string s)
    {
       M(s);
       s.ToString(); //warning CS8602: Dereference of a possibly null reference.
    }
}

sharplab #Resolved

@jcouv

This comment has been minimized.

Copy link
Member Author

jcouv commented Feb 11, 2020

public bool TryGetValue([MaybeNullWhen(false)] out T t) => throw null!;
I would expect that if we had e.g. [NotNullWhen(true)], there would be no warning. Is that the case? Any existing test that shows this?

I think that's covered by NotNullWhenTrue_EnforceInMethodBody and NotNullWhenTrue_EnforceInMethodBody_Warn.
Let me know if those are not the scenarios you had in mind. #Resolved

@jcouv

This comment has been minimized.

Copy link
Member Author

jcouv commented Feb 11, 2020

oh, is it oblivious because we haven't performed nullability analysis at the time we get the overridden symbol?

It's oblivious because we have a bug in type substitution. Fixing that bug introduces a difficult cycle. Filed a follow-up issue. Details are in #41368 #Resolved

@jcouv

This comment has been minimized.

Copy link
Member Author

jcouv commented Feb 11, 2020

Addressed the following comments:

  • I don't see this diagnostic in VerifyDiagnostics
  • Should these trailing comments be deleted?
  • Consider reordering the code for uniformity with the other tests.
  • Are we intentionally missing an override for F6?
  • Are there any tests for hiding members on a derived class?

Fixed/added. Thanks

I think these warnings are pretty serviceable as is, just something to consider.

Leaving as-is for now. We can tweak diagnostics as we go. #Resolved

@jcouv

This comment has been minimized.

Copy link
Member Author

jcouv commented Feb 11, 2020

This PR flagged more issues in bootstrapping that were introduced in master. I'll merge master into this PR and address. #Resolved

{
RoslynDebug.Assert(other is object);

This comment has been minimized.

Copy link
@cston

cston Feb 11, 2020

Member

RoslynDebug.Assert(other is object) [](start = 12, length = 35)

This is a public method so we should handle null. #Resolved

This comment has been minimized.

Copy link
@jcouv

jcouv Feb 11, 2020

Author Member

Thanks, indeed. I'll add a null check (since implementation below would crash on dereference...)


In reply to: 377720266 [](ancestors = 377720266)

@@ -18,12 +19,13 @@ internal MemberRefComparer(MetadataWriter metadataWriter)
_metadataWriter = metadataWriter;
}

public bool Equals(ITypeMemberReference x, ITypeMemberReference y)
public bool Equals([AllowNull] ITypeMemberReference x, [AllowNull] ITypeMemberReference y)

This comment has been minimized.

Copy link
@cston

cston Feb 11, 2020

Member

[AllowNull] ITypeMemberReference [](start = 27, length = 32)

Consider using ITypeMemberReference? rather than [AllowNull]. #Resolved

@@ -18,12 +19,13 @@ internal MethodSpecComparer(MetadataWriter metadataWriter)
_metadataWriter = metadataWriter;
}

public bool Equals(IGenericMethodInstanceReference x, IGenericMethodInstanceReference y)
public bool Equals([AllowNull] IGenericMethodInstanceReference x, [AllowNull] IGenericMethodInstanceReference y)

This comment has been minimized.

Copy link
@cston

cston Feb 11, 2020

Member

[AllowNull] IGenericMethodInstanceReference [](start = 27, length = 43)

IGenericMethodInstanceReference? #Resolved

@@ -17,7 +18,7 @@ internal TypeSpecComparer(MetadataWriter metadataWriter)
_metadataWriter = metadataWriter;
}

public bool Equals(ITypeReference x, ITypeReference y)
public bool Equals([AllowNull] ITypeReference x, [AllowNull] ITypeReference y)

This comment has been minimized.

Copy link
@cston

cston Feb 11, 2020

Member

[AllowNull] ITypeReference [](start = 27, length = 26)

ITypeReference? #Resolved

jcouv added 2 commits Feb 11, 2020
@@ -1,6 +1,6 @@
Microsoft Visual Studio Solution File, Format Version 12.00
# Visual Studio 15
VisualStudioVersion = 15.0.27102.0
# Visual Studio Version 16

This comment has been minimized.

Copy link
@RikkiGibson

RikkiGibson Feb 12, 2020

Contributor

intended?

This comment has been minimized.

Copy link
@jcouv

jcouv Feb 12, 2020

Author Member

Thanks for catching that. My compilers.sln keeps getting updated recently and it's been error-prone to avoid adding it... :(

@jcouv jcouv merged commit 961f261 into dotnet:master Feb 12, 2020
18 checks passed
18 checks passed
WIP Ready for review
Details
license/cla All CLA requirements met.
Details
roslyn-CI Build #20200211.59 succeeded
Details
roslyn-CI (Linux_Test coreclr) Linux_Test coreclr succeeded
Details
roslyn-CI (SourceBuild_Test) SourceBuild_Test succeeded
Details
roslyn-CI (Windows_CoreClr_Unit_Tests debug) Windows_CoreClr_Unit_Tests debug succeeded
Details
roslyn-CI (Windows_CoreClr_Unit_Tests release) Windows_CoreClr_Unit_Tests release succeeded
Details
roslyn-CI (Windows_Correctness_Test) Windows_Correctness_Test succeeded
Details
roslyn-CI (Windows_Desktop_Spanish_Unit_Tests) Windows_Desktop_Spanish_Unit_Tests succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests debug_32) Windows_Desktop_Unit_Tests debug_32 succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests debug_64) Windows_Desktop_Unit_Tests debug_64 succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests release_32) Windows_Desktop_Unit_Tests release_32 succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests release_64) Windows_Desktop_Unit_Tests release_64 succeeded
Details
roslyn-CI (Windows_Determinism_Test) Windows_Determinism_Test succeeded
Details
roslyn-CI (macOS_Test) macOS_Test succeeded
Details
roslyn-integration-CI Build #20200211.59 succeeded
Details
roslyn-integration-CI (VS_Integration debug_async) VS_Integration debug_async succeeded
Details
roslyn-integration-CI (VS_Integration release_async) VS_Integration release_async succeeded
Details
@jcouv jcouv deleted the jcouv:nullable-ohi branch Feb 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.