Switch to using field+auto-prop#76906
Conversation
1b831d0 to
e2b5e9e
Compare
e2b5e9e to
733debf
Compare
|
I think the refactoring should move the property declaration to where the field declaration used to be if it's removing it. Or maybe we need to run follow up refactoring that groups data-backed members together. Having (semi)auto-properties scattered across the type declaration makes it difficult to see what data the object holds onto, weather it has any mutable state, what the value of |
|
I keep forgetting, only the diagnostics from the bootstrap build are going to be interesting here, when evaluating the new field nullability behavior. |
|
https://github.com/dotnet/roslyn/pull/76906/checks?check_run_id=39404423147 These appear to be the interesting diagnostics from the bootstrap build. I thought that the |
0ba173a to
982151e
Compare
|
@JoeRobich ptal. |
| public ISymbol? Symbol { get; } | ||
| public CandidateReason CandidateReason { get; } | ||
| public ImmutableArray<ISymbol> CandidateSymbols => _candidateSymbols.NullToEmpty(); | ||
| public ImmutableArray<ISymbol> CandidateSymbols => field.NullToEmpty(); |
There was a problem hiding this comment.
I never realized there was a setter for this type of property.
| public readonly ISymbol? Symbol = symbol; | ||
|
|
||
| public ImmutableArray<SymbolKindOrTypeKind> PossibleSymbolKinds => _possibleSymbolKinds.NullToEmpty(); | ||
| public ImmutableArray<SymbolKindOrTypeKind> PossibleSymbolKinds { get => field.NullToEmpty(); } = possibleSymbolKinds; |
There was a problem hiding this comment.
I know this PR didn't introduce it, but is there a quirk here that makes NullToEmpty() necessary?
There was a problem hiding this comment.
This is primarily making sure that if someone either passed default to the consturctor or just has a default instance of NameDeclarationInfo that PossibleSymbolKinds still always returns an empty array. It makes it so that teh entire domain you have to think about for PossibleSymbolKinds is non-defualt arrays (so real arrays of 0 to N length). It removes the null/default case as something to ever even have to eb concerned with.
There was a problem hiding this comment.
Practically, we have good hygiene, and this is generally not a concern. But it is something that occasionally arises, and thse patterns help.
| public ExportProvider ExportProvider { get; } | ||
|
|
||
| internal ServerCapabilities ServerCapabilities => _serverCapabilities ?? throw new InvalidOperationException("Initialize has not been called"); | ||
| internal ServerCapabilities ServerCapabilities { get => field ?? throw new InvalidOperationException("Initialize has not been called"); private set; } |
There was a problem hiding this comment.
I like this so much. That it allows field to be nullable before it is initialized. I am thinking of all the places I have nullable fields that are only nullable because they aren't set in the constructor.
| public readonly ISymbol? Symbol = symbol; | ||
|
|
||
| public ImmutableArray<SymbolKindOrTypeKind> PossibleSymbolKinds => _possibleSymbolKinds.NullToEmpty(); | ||
| public ImmutableArray<SymbolKindOrTypeKind> PossibleSymbolKinds { get => field.NullToEmpty(); } = possibleSymbolKinds; |
There was a problem hiding this comment.
Practically, we have good hygiene, and this is generally not a concern. But it is something that occasionally arises, and thse patterns help.
No description provided.