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

Null-forgiving operator #1195

Open
wants to merge 7 commits into
base: draft-v8
Choose a base branch
from
Open

Conversation

Nigel-Ecma
Copy link
Contributor

Null-forgiving operator

  • Fix grammar
  • Specify semantic restrictions
  • Add to precedence table
  • Add to MLR group

Addresses issue #1190

- Fix grammar
- Specify semantic restrictions
- Add to precedence table
- Add to MLR group
- Fix grammar
- Specify semantic restrictions
- Add to precedence table
- Add to MLR group
@Nigel-Ecma
Copy link
Contributor Author

Note: The grammar validation will fail until the validator is updated. The validator is a work-in-progress and there is a mutual dependency between that and this PR. The PR for the validator will follow in the not too distant future…
@BillWagner – I suggest holding off merging this even once approved until a validator PR is good to go as well.

Copy link
Contributor

@jskeet jskeet left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks Nigel.

@KalleOlaviNiemitalo
Copy link
Contributor

The wording does not seem to explain the effect of the null-forgiving operator in this case:

class C {
    static void M() {
        object obj;
        N(out obj); // warning CS8600: Converting null literal or possible null value to non-nullable type.
        N(out obj!); // no warning
    }
    
    static void N(out object? o) { o = null; }
}

standard/expressions.md Outdated Show resolved Hide resolved
@Nigel-Ecma Nigel-Ecma marked this pull request as draft November 1, 2024 21:38
@Nigel-Ecma
Copy link
Contributor Author

Nigel-Ecma commented Nov 1, 2024

@KalleOlaviNiemitalo & @BillWagner – I only came here to fix the grammar! ;-)

Thanks Kalle for the research and further analysis.

It's clear the original spec was never in a released compiler and so we can go straight to allowing value types.

However as Kalle has shown the implemented operator does more than override (aka “forgive”) a compiler’s static analysis determination that a null might be present – it can be a “nullability forgiving operator” and cancel any nullability warnings that a compiler might choose to provide.

I’ve moved the PR to Draft while I determine a suitable spec.

@KalleOlaviNiemitalo
Copy link
Contributor

Compiler behaviour for the null-forgiving operator in the left operand of an assignment expression looks complex:

class C
{
    void M(
        ref object o,
        ref object p)
    {
        // error CS8598
        (o!) = null;

        // warning CS8600, despite ! operator
        (o!, p!) = (1, null);

        // no warning
        (o!, p!) = this;

        // no warning
        Deconstruct(out o!, out p!);
    }
    
    void Deconstruct(
        out object? o,
        out object? p)
    {
        o = null;
        p = null;
    }
}

@KalleOlaviNiemitalo
Copy link
Contributor

KalleOlaviNiemitalo commented Nov 2, 2024

Null-forgiving ! would make perfect sense in the left operand of a compound assignment operator; but the compiler apparently doesn't allow that. The new wording should make sure that the standard doesn't allow that either.

public class C
{
    public static void M()
    {
        C? c = null;
        
        // error CS8598
        (c!) += new C();
        
        // no warning
        c = c! + new C();
    }
    
    public static C operator +(C c1, C c2)
    {
        return new C();
    }
}

@KalleOlaviNiemitalo
Copy link
Contributor

KalleOlaviNiemitalo commented Nov 3, 2024

warning CS8620 looks unjustified here; the type of the argument (p, q) should be (object p, object? q), not (object? p, object? q). It seems the compiler makes the assignment to the tuple (p, q) change the type of p to nullable even before the assignment happens. I suppose, even if it is a bug, it won't be a violation of the C# 8 standard, as compilers are allowed to output extra diagnostics.

public class C
{
    public static void M()
    {
        object p = "";
        object? q = null;
        
        // warning CS8600: Converting null literal or possible null value to non-nullable type.
        // warning CS8620: Argument of type '(object? p, object? q)' cannot be used for parameter 't' of type '(object, object?)' in '(object?, object) C.operator +((object, object?) t, C c)' due to differences in the nullability of reference types.
        (p, q) = (p, q) + new C();     
    }
    
    public static (object?, object) operator +((object, object?) t, C c)
    {
        return (null, "");
    }
}

The same happens with a named method instead of an operator:

public class C
{
    public static void M()
    {
        object p = "";
        object? q = null;
        
        // warning CS8600: Converting null literal or possible null value to non-nullable type.
        // warning CS8620: Argument of type '(object? p, object? q)' cannot be used for parameter 't' of type '(object, object?)' in '(object?, object) C.N((object, object?) t)' due to differences in the nullability of reference types.
        (p, q) = N((p, q));  
    }
    
    public static (object?, object) N((object, object?) t)
    {
        return (null, "");
    }
}

- Added null conditional support
- Distinguished uses of logical negation, `!x`, from null-forgiving, `x!`,
throughout the Standard. Null-forgiving is a non-overloadable psuedo op.
- Added placeholders in places where what the spec is has yet to be decided as the details were not obtained from the LDM notes or other design documents but from the source of one particular compiler. These will be replaced in a PR review posted immediately after this commit, each placeholder will have at least to suggested options to vote on.
- Some typos were fixed or wording changes made, some in areas not directly related to null-forgiving, as they were found during work on this PR.
Copy link
Contributor Author

@Nigel-Ecma Nigel-Ecma left a comment

Choose a reason for hiding this comment

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

In previous discussions on this feature it was known to TG2 that there was one situation where a certain compiler produced a compile time error from a use of the null-forgiving operator, rather than the usual warning.

This seemingly diverged from the design goal that the null-forgiving operator only provides information to a compiler’s optional null state static analysis to effect the analysis and its warnings.

Those discussions led to the decision to include the requirement for the error in the Standard.

Since then a certain compiler’s source code has been reviewed and a total of 5 places where that compiler produces compile time errors from usage of the null-forgiving operator found – none of which appear to be specified in any of the feature design documents.

All 5 places, including the originally known one in §12.8.9, are now marked in this PR with placeholding HTML comments.

This review provides possible text for each placeholder if the particular error is to be included in the Standard and required of all implementations, or the choice of simply removing the placeholder.

Please debate away, either on all 5 errors as a whole or on the individual placeholder comments. TG2 will review the discussion before making final determinations.

standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/unsafe-code.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
@Nigel-Ecma Nigel-Ecma added the meeting: discuss This issue should be discussed at the next TC49-TG2 meeting label Nov 9, 2024
@Nigel-Ecma
Copy link
Contributor Author

Nigel-Ecma commented Nov 10, 2024

General question: Anybody think an example explaining null! would be good and maybe also wish to suggest one, and where to include it, if so?

Copy link
Member

@BillWagner BillWagner left a comment

Choose a reason for hiding this comment

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

Overall this looks good. I had a few comments.

One other question for @jskeet: Typically, we've used hard breaks only for paragraphs. I don't know if the hard returns will create problems for the word converter.

standard/types.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/unsafe-code.md Outdated Show resolved Hide resolved
@Nigel-Ecma
Copy link
Contributor Author

@BillWagner wrote:

One other question for @jskeet: Typically, we've used hard breaks only for paragraphs. I don't know if the hard returns will create problems for the word converter.

The word convertor errors are not due to hard breaks. One of the subclause titles has emphasised text in it and the convertor doesn’t like that. It will be removed or replaced with quotes in the next commit.

@jskeet jskeet marked this pull request as ready for review November 20, 2024 21:04
- Replace placeholders as chosen by meeting
- Make opening text into General subclause, renumber subclauses,
  reword conditionally normative to match revised structure
- Silence the MD2Word convertor
- Ready to merge
- The WordConverter objected to the block comment format, fixed
- Now should be ready to merge...
@Nigel-Ecma Nigel-Ecma removed the meeting: discuss This issue should be discussed at the next TC49-TG2 meeting label Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants