Skip to content

Conversation

@jcouv
Copy link
Member

@jcouv jcouv commented Feb 11, 2020

TL;DR: the goal for this PR is to confirm that some upcoming compiler changes for VS 16.6 are okay. This PR is intended for discussion but not to merge as-is :-)

Tagging @stephentoub @safern for discussion.


Background:

I have been working on compiler changes which adds some warnings as a result of nullability attributes.

For instance:

  • [MaybeNull] string parameter would start with a maybe-null state.
  • [MaybeNull] override T M() cannot override virtual T M()
  • we check the state of [MaybeNullWhen(true)] string parameter
  • we check that methods with [DoesNotReturn] indeed don't return

Jared recommended that I test the change on the runtime repo, and it indeed produced more warnings. Here's a PR that addresses some of the issues flagged and tags some others.

Issues identified so far:

  • some GetHashCode methods are marked as [DisallowNull] and that's causing problems
  • FailFast is marked with [DoesNotReturn], but it looks like it could return in some cases
  • I've tagged a few places where methods with conditional [Maybe/NotNullWhen(...)] attributes seem incorrectly implemented

public bool TryGetValue(T equalValue, out T actualValue)
{
int hashCode = _equalityComparer.GetHashCode(equalValue);
int hashCode = _equalityComparer.GetHashCode(equalValue!);
Copy link
Member Author

@jcouv jcouv Feb 11, 2020

Choose a reason for hiding this comment

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

📝 should we remove the [DisallowNull] on that GetHashCode, or add [DisallowNull] here?
(same question below) #Resolved

Copy link
Member

@stephentoub stephentoub Feb 11, 2020

Choose a reason for hiding this comment

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

In general IEqualityComparer<T>.GetHashCode (e.g. StringComparer.GetHashCode throws ArgumentNullException for null) even if some implementations do, so we can't remove [DisallowNull]. We could consider adding [DisallowNull] to this T equalValue, but then we'd be saying that null wasn't allowed even if the developer provided a comparer implementation that did actually allow null. This is an example where we lack sufficient expressivity. I guess for now it'd be best to add [DisallowNull], and then see what kind of issues that causes; we can remove [DisallowNull] later on, but it'll be harder to add it later on. #Resolved

{
multipleMatches = multipleExports;
// TODO2 singleMatch dould be null when returning true
singleMatch = null!;
Copy link
Member Author

@jcouv jcouv Feb 11, 2020

Choose a reason for hiding this comment

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

📝 the [NotNullWhen(...)] annotation on method seems incorrect #Resolved

Copy link
Member

@stephentoub stephentoub Feb 11, 2020

Choose a reason for hiding this comment

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

Yes, please remove it. Thanks. #Resolved

private static bool TryGetCastFunction(Type genericType, bool isOpenGeneric, Type[] arguments, [NotNullWhen(true)] out Func<Export, object>? castFunction)
{
castFunction = null;
castFunction = null!; // TODO2
Copy link
Member Author

@jcouv jcouv Feb 11, 2020

Choose a reason for hiding this comment

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

📝 the [NotNullWhen(...)] annotation on method seems incorrect #Resolved

Copy link
Member

@stephentoub stephentoub Feb 11, 2020

Choose a reason for hiding this comment

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

Yes, please remove it. The dangers of the compiler not having checked those annotations ;) #Resolved

if (this.IsGeneric())
{
singleMatch = null;
singleMatch = null!; // TODO2
Copy link
Member Author

@jcouv jcouv Feb 11, 2020

Choose a reason for hiding this comment

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

📝 the [NotNullWhen(...)] annotation on method seems incorrect #Resolved

Copy link
Member

@stephentoub stephentoub Feb 11, 2020

Choose a reason for hiding this comment

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

Ditto #Resolved

int IComparer<DebugInfo>.Compare([AllowNull] DebugInfo d1, [AllowNull] DebugInfo d2)
{
if (d1.Index > d2.Index) return 1;
if (d1!.Index > d2!.Index) return 1; // TODO2
Copy link
Member Author

@jcouv jcouv Feb 11, 2020

Choose a reason for hiding this comment

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

📝 IComparer declares that it can compare null inputs, but this implementation doesn't handle. #Resolved

Copy link
Member

@stephentoub stephentoub Feb 11, 2020

Choose a reason for hiding this comment

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

}
WriteAssert(stackTrace, message, detailMessage);
FailCore(stackTrace, message, detailMessage, "Assertion failed.");
#nullable disable
Copy link
Member Author

@jcouv jcouv Feb 11, 2020

Choose a reason for hiding this comment

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

📝 without this suppression, the compiler points out that this method could return normally. Looking at the implementation of FailCore, I think it has the same problem.
We need to discuss what is the best way to suppress this warning. Maybe a more fined-grained suppression on a specific diagnostic would be better than the coarse #nullable disable I used... #Resolved

Copy link
Member

@stephentoub stephentoub Feb 11, 2020

Choose a reason for hiding this comment

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

You're nullable disabling/enabling around the closing brace?

Regardless, yes, it's possible for someone to implement Fail in a way that still returns, but that goes against the purpose of the method. If we held true to the possibility that someone did that, then we'd need to remove [DoesNotReturnIf(false)] from Debug.Assert and all such methods, which would be terrible. #Resolved

Copy link
Member

@safern safern Feb 11, 2020

Choose a reason for hiding this comment

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

Can't we add DoesNotReturn to FailCore? #Resolved

Copy link
Member Author

@jcouv jcouv Feb 11, 2020

Choose a reason for hiding this comment

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

@safern FailCore has return statements, so the same warning "method should not return" would be produced there if we add [DoesNotReturn]. #Resolved

public bool Equals([AllowNull] RoAssemblyName other)
{
if (Name != other.Name)
if (Name != other!.Name) // TODO2
Copy link
Member Author

@jcouv jcouv Feb 11, 2020

Choose a reason for hiding this comment

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

📝 not sure what to do here #Resolved

Copy link
Member

@safern safern Feb 11, 2020

Choose a reason for hiding this comment

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

why did we need to make it nullable? #Resolved

Copy link
Member

@stephentoub stephentoub Feb 11, 2020

Choose a reason for hiding this comment

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

This is an internal type, and presumably it's not expecting Equals to be used with null, even though it implements an interface that allows it. Seems like the right change is to remove this ! and ? and suppress the warning on the Equals method signature. #Resolved

Copy link
Member Author

@jcouv jcouv Feb 11, 2020

Choose a reason for hiding this comment

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

To implement IEquatable you must be compatible with the signature bool Equals([AllowNull] T other); #Resolved

Copy link
Member

@stephentoub stephentoub Feb 11, 2020

Choose a reason for hiding this comment

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

To implement IEquatable you must be compatible with the signature bool Equals([AllowNull] T other);

I understand that. So I'm saying use #pragma warning disable whatever to suppress the warning. #Resolved

{
OperationResult result;
int hashCode = origin.EqualityComparer.GetHashCode(item);
int hashCode = origin.EqualityComparer.GetHashCode(item!);
Copy link
Member

@stephentoub stephentoub Feb 11, 2020

Choose a reason for hiding this comment

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

But now it seems like that would actually not just be for TryGetValue, but really all operations on the hash set, at which point we'd be better off using a notnull constraint? Maybe instead of changing the annotations, open an issue about it and keep the ! changes you have here. #Resolved

Copy link
Member

@stephentoub stephentoub Feb 11, 2020

Choose a reason for hiding this comment

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

(And actually, the right answer is probably to do a null check prior to calling GetHashCode, as we do in HashSet<T>.) #Resolved

}

public bool Equals(T x, T y)
public bool Equals([AllowNull] T x, [AllowNull] T y)
Copy link
Member

@safern safern Feb 11, 2020

Choose a reason for hiding this comment

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

Since T is constraint to class, can we instead annotate T as T? ? #Resolved

Copy link
Member Author

@jcouv jcouv Feb 11, 2020

Choose a reason for hiding this comment

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

Indeed. I'd missed that. Thanks #Resolved

Copy link
Member Author

@jcouv jcouv Feb 12, 2020

Choose a reason for hiding this comment

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

Hum, it looks like this file is compiled into more than one project. One with nullability enabled and one without. Reverted to use the attribute. #Resolved

Copy link
Member

@stephentoub stephentoub Feb 12, 2020

Choose a reason for hiding this comment

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

Can you use the annotation and just add #nullable enable at the top of the file as well? That's what we've done in other cases like this, where we've not yet gotten to every project that includes it. #Resolved

{
Add(comparer != null ? comparer.GetHashCode(value) : (value?.GetHashCode() ?? 0));
// TODO2
Add(comparer != null ? comparer.GetHashCode(value!) : (value?.GetHashCode() ?? 0));
Copy link
Member

@stephentoub stephentoub Feb 11, 2020

Choose a reason for hiding this comment

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

As with the earlier examples with ImmutableHashSet, we should probably fix this implementation. e.g. today if you wrote code like:

#nullable enable
using System;

class Program
{
    static void Main(string[] args)
    {
        HashCode c = default;
        c.Add(null, StringComparer.Ordinal);
    }
}

it will blow up. Seems like this line should instead be:

Add(value is null ? 0 :
    comparer != null ? comparer.GetHashCode(value) :
    value.GetHashCode());

or something like that. #Resolved

TManager? localManager; // Use register for null comparison rather than byref
manager = localManager = memory.GetObjectStartLength(out _, out _) as TManager;
return localManager != null;
return localManager != null; // TODO2
Copy link
Member

@stephentoub stephentoub Feb 11, 2020

Choose a reason for hiding this comment

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

What's wrong here? #Resolved

Copy link
Member Author

@jcouv jcouv Feb 11, 2020

Choose a reason for hiding this comment

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

The compiler doesn't know that manager == localManager, so it looks like we may be returning with a null manager when this method returns true.
We could fix that by doing return manager != null;, but that defeats the purpose indicated by above comment ("Use register for null comparison rather than byref"). #Closed

Copy link
Member

@stephentoub stephentoub Feb 11, 2020

Choose a reason for hiding this comment

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

Ah, so this TODO was about improving the compiler :)

For the purposes of this change, just suppress the warning with whatever syntax is least obtrusive. #Resolved

Debug.Assert(length >= 0);

if (localManager == null)
if (localManager == null) // TODO2
Copy link
Member

@stephentoub stephentoub Feb 11, 2020

Choose a reason for hiding this comment

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

What's wrong here? #Resolved

Copy link
Member Author

@jcouv jcouv Feb 11, 2020

Choose a reason for hiding this comment

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

Ditto #Closed


using System.Diagnostics.CodeAnalysis;
using System.Runtime.CompilerServices;
#nullable enable
Copy link
Member

@stephentoub stephentoub Feb 13, 2020

Choose a reason for hiding this comment

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

Nit: mind moving this above the usings? #Resolved

public bool TryGetValue(T equalValue, out T actualValue)
{
int hashCode = _equalityComparer.GetHashCode(equalValue);
int hashCode = equalValue is object ? _equalityComparer.GetHashCode(equalValue) : 0;
Copy link
Member

@stephentoub stephentoub Feb 13, 2020

Choose a reason for hiding this comment

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

We've generally not used "is object" as a null check.

Suggested change
int hashCode = equalValue is object ? _equalityComparer.GetHashCode(equalValue) : 0;
int hashCode = equalValue != null ? _equalityComparer.GetHashCode(equalValue) : 0;
``` #Resolved

{
OperationResult result;
int hashCode = origin.EqualityComparer.GetHashCode(item);
int hashCode = item is object ? origin.EqualityComparer.GetHashCode(item) : 0;
Copy link
Member

@stephentoub stephentoub Feb 13, 2020

Choose a reason for hiding this comment

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

Suggested change
int hashCode = item is object ? origin.EqualityComparer.GetHashCode(item) : 0;
int hashCode = item != null ? origin.EqualityComparer.GetHashCode(item) : 0;
``` #Resolved

{
var result = OperationResult.NoChangeRequired;
int hashCode = origin.EqualityComparer.GetHashCode(item);
int hashCode = item is object ? origin.EqualityComparer.GetHashCode(item) : 0;
Copy link
Member

@stephentoub stephentoub Feb 13, 2020

Choose a reason for hiding this comment

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

Suggested change
int hashCode = item is object ? origin.EqualityComparer.GetHashCode(item) : 0;
int hashCode = item != null ? origin.EqualityComparer.GetHashCode(item) : 0;
``` #Resolved

private static bool Contains(T item, MutationInput origin)
{
int hashCode = origin.EqualityComparer.GetHashCode(item);
int hashCode = item is object ? origin.EqualityComparer.GetHashCode(item) : 0;
Copy link
Member

@stephentoub stephentoub Feb 13, 2020

Choose a reason for hiding this comment

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

Suggested change
int hashCode = item is object ? origin.EqualityComparer.GetHashCode(item) : 0;
int hashCode = item != null ? origin.EqualityComparer.GetHashCode(item) : 0;
``` #Resolved

{
if (second == null)
{
Debug.Assert(value is object);
Copy link
Member

@stephentoub stephentoub Feb 13, 2020

Choose a reason for hiding this comment

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

How do we know this assert is valid? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

The two callers get their values from ComposablePartDefinition.TryGetExports. That method can return with

  • both singleMatch and multipleMatches being null (return false;, but then FastAppendToListAllowNulls won't be called because of calling pattern, copied below),
  • or multipleMatches being set and returning true,
  • or singleMatch being set and returning true.

singleMatch corresponds to parameter value and multipleMatches corresponds to parameter second here.

Calling pattern:

                    if (part.TryGetExports(definition, out Tuple<ComposablePartDefinition, ExportDefinition>? singleMatch, out IEnumerable<Tuple<ComposablePartDefinition, ExportDefinition>>? multipleMatches))
                    {
                        exports = exports.FastAppendToListAllowNulls(singleMatch, multipleMatches);
                    }

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

foreach (T t in list)
{
h ^= (h << 5) ^ cmp.GetHashCode(t);
if (t is object)
Copy link
Member

@stephentoub stephentoub Feb 13, 2020

Choose a reason for hiding this comment

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

Suggested change
if (t is object)
if (t != null)
``` #Resolved

int IComparer<DebugInfo>.Compare(DebugInfo d1, DebugInfo d2)
int IComparer<DebugInfo>.Compare(DebugInfo? d1, DebugInfo? d2)
{
Debug.Assert(d1 is object && d2 is object);
Copy link
Member

@stephentoub stephentoub Feb 13, 2020

Choose a reason for hiding this comment

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

Suggested change
Debug.Assert(d1 is object && d2 is object);
Debug.Assert(d1 != null && d2 != null);
``` #Resolved

Debug.Assert(_comparer != null);
return _comparer.GetHashCode(x.Value);
T value = x.Value;
return value is null ? 0 : _comparer.GetHashCode(value);
Copy link
Member

@stephentoub stephentoub Feb 13, 2020

Choose a reason for hiding this comment

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

Suggested change
return value is null ? 0 : _comparer.GetHashCode(value);
return value != null ? 0 : _comparer.GetHashCode(value);
``` #Resolved

throw new ArgumentException(SR.Format(SR.DeserializeWrongType, type, value.GetType()));
}

[DoesNotReturn]
Copy link
Contributor

Choose a reason for hiding this comment

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

I am curious, how did you catch this typo?

Copy link
Member Author

Choose a reason for hiding this comment

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

A new version of the compiler checks method bodies to see that they honor the nullability attributes that they declare in their API. This method has return statements, so it was flagged ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

@jcouv
Copy link
Member Author

jcouv commented Feb 14, 2020

I believe this is ready to merge/squash if I can get the required sign-off(s). Thanks

@stephentoub
Copy link
Member

Thanks, Julien.

@stephentoub stephentoub merged commit 0f834db into dotnet:master Feb 14, 2020
@jcouv jcouv deleted the nullable-ohi branch February 14, 2020 17:43
@jcouv
Copy link
Member Author

jcouv commented Feb 14, 2020

Thanks!

@jcouv jcouv self-assigned this Feb 14, 2020
@jcouv jcouv added this to the 5.0 milestone Feb 14, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants