Use non-null initial state for null-resilient properties in constructors - #84991
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
RikkiGibson
changed the title
9cb71ca to
f7b09e4
Compare
There was a problem hiding this comment.
🟡 Changes recommended
There are verified issues in NullableWalker (debug assertion side effects, accessibility assumptions, and malformed XML doc comments) that should be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 2 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core nullable flow analysis slot/ownership behavior in the compiler, which is regression-prone and warrants final review by a domain expert with full CI signal.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
| public C(D3 d3) // 3 | ||
| { | ||
| Prop1 = null; | ||
| Prop2 = null; // 4 |
There was a problem hiding this comment.
Should we also read the properties after assignment in these new tests to observe any possible nullability warnings there?
| return builder.ToImmutableAndFree(); | ||
| } | ||
|
|
||
| public static ImmutableArray<TResult> SelectManyAsArray<TItem, TArg, TResult>(this IReadOnlyCollection<TItem>? source, Func<TItem, TArg, OneOrMany<TResult>> selector, TArg arg) |
There was a problem hiding this comment.
Should we have an overload that takes ImmutableArray receiver too? Looks like this is currently being called on ImmutableArray receivers, causing boxing.
| case PropertySymbol { IsRequired: true }: | ||
| case PropertySymbol { IsRequired: true } requiredProperty when GetAccessibleBackingField(requiredProperty, method.ContainingType) is null: | ||
| // Visit a required property, unless it has a backing field which is accessible in the current context, | ||
| // in which case we expect the field to be visited in another iteration of this loop. |
There was a problem hiding this comment.
It looks like the field can be skipped in some cases (in if (IsPropertyOutputMoreStrictThanInput(prop))). For example,
#nullable enable
using System.Diagnostics.CodeAnalysis;
public class C
{
[AllowNull]
public required string P { get; set; }
[SetsRequiredMembers]
public C()
{
P.ToString();
}
}correctly reported a warning on P.ToString(); before this PR, but does not report it after.
There was a problem hiding this comment.
The thing is, the warning is also missing before this PR, when required and [SetsRequiredMembers] are both deleted. So, I could see about putting the warning in both scenarios. Hopefully there is now a single point of control for doing that.
There was a problem hiding this comment.
If you are comfortable with it, I'd like to address the AllowNull issue more holistically in a follow-up. Basically, I think that we could arrive at an overall more desirable behavior by mostly just deleting a little more implementation code. But, there is test churn involved.
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core nullable flow analysis and slot-sharing behavior in NullableWalker, which is high-risk and warrants final review by a compiler maintainer despite added tests.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/Compilers/CSharp/Portable/FlowAnalysis/LocalDataFlowPass.cs:68
- There’s a small typo in the updated XML comment: “struct constructs” should be “struct constructors”.
src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs:574 - The DEBUG assert in TryGetVariable() is intended to enforce that the slot lookup uses the owning symbol, but tuple equality here uses Symbol.Equals (which can be overridden for non-reference equality). That can allow a different-but-equal symbol to slip through, defeating the purpose of the assert.
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
ImmutableArrayExtensions.SelectManyAsArray has a malformed addition (missing method signature) that will break compilation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
A new public extension method overload has incomplete XML documentation (missing <param name="arg">), which should be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
| if (array.Length == 0) | ||
| return []; | ||
|
|
||
| var builder = ArrayBuilder<TResult>.GetInstance(); |
There was a problem hiding this comment.
Interesting that we are not setting the capacity here like we are in the IReadOnlyCollection equivalent. But this matches the other ImmutableArray extensions around, so I guess it's fine.
|
/pr-val adcc3fb |
|
View PR Validation Run triggered by @RikkiGibson Parameters
|
|
CloudBuild is failing in insertion, it looked like a locked file issue, unlikely to be related to this change. I re-queued. |
|
VS build is clean. |
Closes #77991
2 principles in this change:
fieldkeyword use a shared slot in constructors. The reason is that there is no strong connection between their respective flow states in this situation. Our analysis decided that the field's value doesn't affect the nullability of the get accessor.This also adds verification, that when a slot is shared, that we only ever lookup the slot for a variable using the "owning" symbol. Otherwise we can get really painful bugs where we might have a slot for both "owning" and "non-owning" when we meant to share a single one, or, we can report no state is tracked because we forgot to use the "owning" symbol for the lookup.
Microsoft Reviewers: Open in CodeFlow