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

[Work items] Nullable reference types #22152

Closed
cston opened this issue Sep 15, 2017 · 3 comments
Closed

[Work items] Nullable reference types #22152

cston opened this issue Sep 15, 2017 · 3 comments

Comments

@cston
Copy link
Member

cston commented Sep 15, 2017

Preview 2:

Non-blocking:

Productivity Migrated to project https://github.com/dotnet/roslyn/projects/43

Work related to project-system:

  • project-system API for CPS Add NullableReferenceTypes property project-system#4277
  • project-system API for csproj/legacy
  • add drop-down in UI
  • SDK work to set default value
  • Code fixer to set NullableReferenceTypes project property (setting Project.Configuration.Properties["NullableReferenceTypes"] and declaring that "NullableReferenceTypes" BooleanProperty)

Preview 1:

@jcouv jcouv added this to the 15.later milestone Sep 21, 2017
@jcouv
Copy link
Member

jcouv commented Oct 5, 2017

Championed issue: dotnet/csharplang#36

Notes on project property:

  • Instructions to test project-system and roslyn together: https://github.com/dotnet/project-system/blob/master/docs/repo/getting-started.md#deploying-to-a-different-hive

  • project.ConfigurationManager.ConfigurationRow(configurationName).Item(1).Properties["NullableReferenceTypes"]

  • Revisit rules for un-initialized fields? (see thread with Oren)

  • should we check implementations of methods that use [NotNullWhenTrue]?

  • Test #nullable within #ifdef directive

  • Test with Pair or tuple: F((maybeNull, oblivious), (maybeNull, nonNull), (maybeNull, maybeNull))

  • Oblivious means "assume the best"

  • What are the scoping rules of #nullable in using aliases?

  • How does the public API work for nullable value types?

  • scripting

  • Document breaking change: if you were defining a NonNullTypes attribute, then the compiler will now reject it.

  • External annotations and determinism

  • LDM: should we warn when assigning to oblivious? MOblivious(null);

  • LDM: var x = oblivious; we probably need to keep x as oblivious here. Or do we assume ??

  • LDM: what do we emit for string? in C# 7 (new compiler)? I assume the compiler should produce an error.

  • Verify older compilers consuming newer library that uses the new constraints

  • LDM: is T? where T : class? allowed or meaningful? (error CS8627)

  • block T? if T is unconstrained

  • verify behavior with LangVersion 7.3 or no [module: NonNullTypes].

Dogfood experience:

  • I want to review all the public APIs. How do I do that?
    • PublicAPI analyzer/fixer should be updated with nullability.
  • Problem with linked source files (CS0453)
  • Need a way to dump all the APIs which an assembly depends on (so you can think about annotating them)
  • FixOnce for less trivial cases (PR Add DeclareAsNullable refactoring #26661)
    • local == null and local != null should offer me to fix the declaration of local
  • Highlight oblivious methods and offer dialog to annotate, or write a tool to annotate BCL.
  • Need to clarify guidelines: my current assumption is that if a null parameter guarantees to throw, then the parameter should be marked as non-nullable.
  • Confirm behavior for [EnsuresTrues] on a conditional method like Debug.Assert
  • I think we need URTANN
  • CodeAnalysis warnings did not contain that many W warnings (gist)
  • FixAll for trivial cases (PR Add a DeclareAsNullable fixer #26630)
    • return null
    • parameter with default value null
    • local declared with null
  • It's useful to have different annotation for getter and for setter. Maybe always set non-null. But before the first set, get could return null. (Need annotation for nullable property with non-nullable setter #26621)
  • External attribute annotation for IsNullOrEmpty and Debug.Assert (PR Use [NotNullWhenFalse] and [EnsuresNotNull] to drive nullability analysis #26656)

Some various notes from LDM discussions and others:

  • Compiler or analyzer warning on using ? in a scope where [NonNullTypes(false)] (warning CS8632)

  • [module: NonNullTypes] is confusing (most developers aren't aware of module: target.

  • Is !! ok? (answer: yes)

  • Confirm whether we want to allow out x! (currently allowed) or x! = y (currently disallowed).

  • How do you version APIs that have nullability or flow annotations?

  • Analyzer for bad usage of attributes, or to recommend using them?

  • Nullability in Razor pages

  • Once the feature can be turned on/off independently of LangVersion (issue Add flag to control nullable feature #26692), we should ensure UpgradeProject experience is still good.

  • Can we bootstrap the BCL annotations using existing CodeContracts?

  • Should we warn if you use the annotations, but the flag is off? Maybe grey them out? (Champion "Nullable reference types" (16.3, Core 3) csharplang#36 (comment))

  • How do we track nullable structs? S? s = .... Does checking .HasValue affect flow analysis?

  • Does ! produce a result that is oblivious or non-null? (! stickiness) (answer: non-null)

  • Should referencing an un-annotated library produce a warning? (answer: no longer applicable)

  • Should MakeArray(new C()) produce a result that is Array<C> or Array<C?>? How do we know that that method doesn't add nulls? On the other hand, how does a method indicate that it returns a non-nullable when the type argument is non-nullable? (maybe we need a constraint: Array<T2> MakeArray(T element) where ...)

  • Update templates (so new projects have the feature turned on by default)

  • What should IDE show at various points in the method? Does having the feature on or off affect what is shown?

  • Could users annotate some methods to reset flow analysis? (for example, p.ChangeMiddleNameToNull())

  • Are we going to apply similar flow analysis and warnings to nullable value types?

  • How are we going to add annotations to BCL, and older frameworks in particular?

  • Lambda with multiple returns, one is a non-null object and the other is nullable local which flow analysis knows isn't null. What is the return type? Same question for other "best type" scenarios (ternary, method type inference).

  • Should nullability be exposed in APIs (IOperation) or UI?

  • Should there be a warning on useless !? (maybe grey it out too?)

  • Should test that nullable annotations from a metadata reference are always displayed in QuickInfo (even in a C# 7 project).

  • Probably this already works, but we should check the intersection between nullability and patterns/switch features: case int? x: // error already, case string? y: // should also be error, case string z: // should be non-null, incomplete switch expression that doesn't handle null should warn (even if input is considered non-null?).

  • string.IsNullOrEmpty should inform flow analysis

    • We'll probably want to annotate some key APIs (like TryGetValue)
    • I assume we'll need some attribute to annotate such APIs. Could the compiler generate the attribute, or an analyzer suggest it, by inspecting the body of the method?
  • TryGetValue should inform flow analysis

  • string? x = null; x = nonNull; var y = new [] { x }; What is the type of y?

  • string? x = null; x = nonNull; var y = MakeArray(x, 10); What is the type of y?

  • Should we warn if conditionally accessing a value that is known to be non-null? I suspect we don't. (Warn if a value is never or always null #22743)

  • x is null should inform flow analysis (Pattern test should inform nullability #26745)

  • From a customer:

class MyClass
{
    private string m_IdAsString; // warning CS8618: Non-nullable field 'm_IdAsString' is uninitialized.
    public Guid Id
    {
        get { return new Guid( m_IdAsString ); }
        set { m_IdAsString = value.ToString(); }
    }
    public MyClass()
    {
        Id = Guid.Empty;
    }
}
  • Variance with out variables
    static void F(out string x) => throw new Exception();
    static void G(out IEnumerable<string> y) => throw new Exception();

    F(out string? x);              // warning?
    G(out IEnumerable<string?> y); // warning?
  • Inferred null state from !
    static void F<T>(T x, out T y) => throw new Exception();

    string? x = null;
    string y;
    F(x, out y);  // warning
    F(x!, out y); // warning?
    static void F(object? x)
    {
        object y;
        y = x;        // warning: `x` may be null
        y.ToString(); // warning?
        y = x!;       // no warning
        y.ToString(); // warning?
    }
  • Warnings for unnecessary !
    static void F(object? x)
    {
        if (x == null) return;
        x!.ToString(); // warning: unnecessary `!`?
        // maybe the IDE offers to remove uncessary ! operators?
    }
  • How to tell the nullable walker about flow of various methods (in most general sense, could be represented by a code snippet):

    • ThrowIfNull
    • IsNullOrEmpty
    • Debug.Assert(x 1= null)
    • FirstOrDefault
  • Tracking null state of non-nullable locals

  • Constraining T to object or class

  • VSIX feed: https://dotnet.myget.org/F/roslyn-nonnull/vsix/

@Joe4evr
Copy link

Joe4evr commented Oct 14, 2017

string.IsNullOrEmpty should inform flow analysis

Nit: Should probably add .IsNullOrWhitespace while you're at it.

@jaredpar jaredpar modified the milestones: 15.6, 16.0 Jan 5, 2018
@jaredpar jaredpar added the Bug label Sep 5, 2018
@jaredpar jaredpar added Discussion and removed Bug labels Apr 9, 2019
@jcouv
Copy link
Member

jcouv commented May 11, 2019

I looked at the remaining issues. Extracted one or two as discrete issues.
I think we can now close this umbrella issue because we now use the Nullable Board.

@jcouv jcouv closed this as completed May 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

4 participants