Exclude identifiers in misplaced attributes from identifier map

[jcouv]

Fixes #73905

For context:

• buildIdentifierMapOfBindIdentifierTargets builds a map of identifiers that we expect to bind
• adjustIdentifierMapIfAny updates the map to record which identifiers we actually bound
• assertBindIdentifierTargets checks the resulting map (already ignored identifiers in presence of binding errors, but now also skips identifiers in presence of warning for misplaced attributes, which causes attributes not to be bound)
• those checks are meant to align with logic in SynthesizedPrimaryConstructor.GetCapturedParameters

[AlekseyTs]

case AttributeListSyntax:

I guess I should have added a comment that logic in this lambda is based on Binder.IdentifierUsedAsValueFinder.CheckIdentifiersInNode.childrenNeedChecking. It can be more permissive (i.e. allow us to dive into more nodes), but should not be more restrictive. Since childrenNeedChecking has the following case:

                        case AttributeListSyntax:
                            // References in attributes, if any, are errors
                            // Skip them
                            return false;

I am fine with adding a similar case here with the same comment/explanation. Given the sated goal "Predict all identifiers in the body that should go through Binder.BindIdentifier method", putting it into the bucket of "uninteresting" nodes is somewhat misleading because it contains identifiers that go through Binder.BindIdentifier.

At the same time, I would prefer an alternative approach to suppressing the assert. The one that will allow assertBindIdentifierTargets to be closer to achieving the stated goal (quoted above).

assertBindIdentifierTargets already disables asserts in presence of errors since there is no guaranty that we bind all the identifiers that are supposed to be bound. The scenarios that we are dealing with are actually error scenarios, we skip binding the attributes, but report a warning instead. That is why assertBindIdentifierTargets fails to disable the check. I suggest to fix this flaw instead. I'll make a specific suggestion at the relevant location.
#Closed

[AlekseyTs]

                            if (flags == 1)

It looks like adding the following code at the end of body of this if suppresses the assert:

if (id.Ancestors(ascendOutOfTrivia: false).OfType<AttributeListSyntax>().Any() &&
    diagnostics.DiagnosticBag!.AsEnumerable().Any(d => d.Code == (int)ErrorCode.WRN_AttributeLocationOnBadDeclaration))
{
    continue;
}
``` #Closed

---
Refers to: src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs:2112 in a3ad201. [](commit_id = a3ad201c5ad78ae8f1229417d4d44d00688f1658, deletion_comment = False)

[AlekseyTs]

if (current is ContextualAttributeBinder)

If we end up keeping an additional logic here (which wouldn't be necessary with the alternative approach), I would prefer it to be a syntactic check based on id. #Closed

[AlekseyTs]

Done with review pass (commit 1)

[jcouv]

@dotnet/roslyn-compiler for second review. Thanks

[AlekseyTs]

LGTM (commit 2)

[AlekseyTs]

While merging, consider adjusting commit's title and comment to align them with the latest revision.

[jcouv]

While merging, consider adjusting commit's title and comment to align them with the latest revision.

Yup. Already done :-)

[jcouv]

@dotnet/roslyn-compiler for second review. Thanks