Skip to content

Comments

Fix binding of nameof of Color Color#82477

Open
jjonescz wants to merge 4 commits intodotnet:mainfrom
jjonescz:82474-NameOf-ColorColor
Open

Fix binding of nameof of Color Color#82477
jjonescz wants to merge 4 commits intodotnet:mainfrom
jjonescz:82474-NameOf-ColorColor

Conversation

@jjonescz
Copy link
Member

Fixes #82474.

}
}

bool useType = !Compilation.IsFeatureEnabled(MessageID.IDS_FeatureInstanceMemberInNameof);
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 20, 2026

Choose a reason for hiding this comment

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

bool useType = !Compilation.IsFeatureEnabled(MessageID.IDS_FeatureInstanceMemberInNameof);

Is there an observable difference from this check? I cannot tell from the added tests. #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the behavior for the test scenarios when the feature is not enabled?

Copy link
Member Author

@jjonescz jjonescz Feb 20, 2026

Choose a reason for hiding this comment

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

Yes, without this we would bind always as value, not type, which would result in langversion error for IDS_FeatureInstanceMemberInNameof in C# 11 or lower, but since we pass BindingDiagnosticsBag.Discarded below, the error would be surfaced as "Unable to determine specific cause of the failure." during emit.

Copy link
Member Author

Choose a reason for hiding this comment

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

In other words, without this change, the tests would fail with unexpected diagnostic "Unable to determine specific cause of the failure." in C# 11.

Copy link
Contributor

Choose a reason for hiding this comment

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

the error would be surfaced as "Unable to determine specific cause of the failure." during emit.

Because?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the node would have HasErrors = true, those errors wouldn't be in any diagnostics bag, but methodCompiler._globalHasErrors would be true, hitting this:

// If we are trying to emit and there's an error without a corresponding diagnostic (e.g. because
// we depend on an invalid type or constant from another module), then explicitly add a diagnostic.
// This diagnostic is not very helpful to the user, but it will prevent us from emitting an invalid
// module or crashing.
if (moduleBeingBuiltOpt != null && (methodCompiler._globalHasErrors || moduleBeingBuiltOpt.SourceModule.HasBadAttributes) && !diagnostics.HasAnyErrors() && !hasDeclarationErrors)
{
var messageResourceName = methodCompiler._globalHasErrors ? nameof(CodeAnalysisResources.UnableToDetermineSpecificCauseOfFailure) : nameof(CodeAnalysisResources.ModuleHasInvalidAttributes);
diagnostics.Add(ErrorCode.ERR_ModuleEmitFailure, NoLocation.Singleton, ((Cci.INamedEntity)moduleBeingBuiltOpt).Name!,
new LocalizableResourceString(messageResourceName, CodeAnalysisResources.ResourceManager, typeof(CodeAnalysisResources)));
}

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 1)

methodGroup.Flags,
methodGroup.FunctionType,
receiverOpt: ReplaceTypeOrValueReceiver(methodGroup.ReceiverOpt, useType: false, BindingDiagnosticBag.Discarded), //only change
receiverOpt: ReplaceTypeOrValueReceiver(methodGroup.ReceiverOpt, useType: useType, BindingDiagnosticBag.Discarded), //only change
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 20, 2026

Choose a reason for hiding this comment

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

useType

Could you please investigate how TypeOValueReceiver was handled for the problematic scenario prior to #80978? #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume it remained in the tree, but was never used during emit or lowering. Is this correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is correct, BoundTypeOrValueExpression node remained in the tree.

@jjonescz jjonescz marked this pull request as ready for review February 20, 2026 15:50
@jjonescz jjonescz requested a review from a team as a code owner February 20, 2026 15:50
}
class D
{
public void M() { }
Copy link
Contributor

Choose a reason for hiding this comment

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

public void M() { }

Consider also testing with static member

methodGroup.Flags,
methodGroup.FunctionType,
receiverOpt: ReplaceTypeOrValueReceiver(methodGroup.ReceiverOpt, useType: false, BindingDiagnosticBag.Discarded), //only change
receiverOpt: ReplaceTypeOrValueReceiver(methodGroup.ReceiverOpt, useType: useType, BindingDiagnosticBag.Discarded), //only change
Copy link
Contributor

Choose a reason for hiding this comment

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

useType

Can we always pass true here?

methodGroup.Flags,
methodGroup.FunctionType,
receiverOpt: ReplaceTypeOrValueReceiver(methodGroup.ReceiverOpt, useType: false, BindingDiagnosticBag.Discarded), //only change
receiverOpt: ReplaceTypeOrValueReceiver(methodGroup.ReceiverOpt, useType: useType, BindingDiagnosticBag.Discarded), //only change
Copy link
Contributor

Choose a reason for hiding this comment

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

BindingDiagnosticBag.Discarded

It looks like always discarding diagnostics here is fragile. We should try switching to boundArgument.HasAnyErrors ? BindingDiagnosticBag.Discarded : diagnostics. Also, please check other call sites of this helper that unconditionally discard diagnostics when we are not in an obvious error recovery mode. We might want to take a close look at those and quite possibly adjust as well. Also, please review other changes in #80978 that unconditionally discard diagnostics.

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 1)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

nameof of Color Color in field initializer results in "Unable to determine specific cause of the failure."

2 participants