[semi-auto-props]: Avoid using IsAutoProperty when possible

[Youssef1313]

Test plan: #57012

[Youssef1313]

Will this require extra binding? 😕

[333fred]

It will require some amount of binding, certainly, but I think it won't require fully binding the body. We'll just need to check to see whether any identifiers named field are used without a field being in scope.

[Youssef1313]

We'll just need to check to see whether any identifiers named field are used without a field being in scope.

Any pointer on how can I see if there are identifiers named field without needing to fully bind the bind? Introducing a new BinderFlag for example? (I know it's unlikely we want to introduce a binder flag for this, but can't think of the correct way to do it)

[333fred]

My thought is that we can walk the binder chain, looking for anything in scope named field. IE, locals or fields. I do think we should ask the LDM what this code means though:

int Prop
{
    set
    {
        field = value;
        {
            int field = 0; // Does this shadow the backing field? Or is there an error reported somewhere?
        }
    }
}

[Youssef1313]

@333fred Do I need to open an issue for that in dotnet/csharplang?

cc @CyrusNajmabadi

[AlekseyTs]

Do I need to open an issue for that in dotnet/csharplang?

I think there is no design question here. The specification is clear: "The field identifier is only considered the field keyword when there is no existing symbol named field in scope at that location." We can certainly have different implementation strategies, but for now I would stick with the simplistic approach of simply binding the body. Anything else is more complexity and more risk.

[333fred]

@333fred Do I need to open an issue for that in dotnet/csharplang?

Yes, let's get that in an issue, or in the spec.

[Youssef1313]

@AlekseyTs For review.

[333fred]

LGTM (commit 4)

[AlekseyTs]

Done with review pass (commit 4)

[AlekseyTs]

@Youssef1313 Please use meaningful names for PRs. Each PR has a goal and that goal should be reflected in the title.

[AlekseyTs]

I am not sure if using the same implementation as for AllowInitializer is the right thing to do. Earlier I was suggesting the following: "Could this be as simple as checking if getter exists and is auto-implemented instead?" It feels like that logic will exactly match the name of the property that we are replacing, IsAutoPropertyWithGetAccessor. I could imagine even more relaxed implementation. Something like

_getMethod?.BodyShouldBeSynthesized == true ||
                    _setMethod?.BodyShouldBeSynthesized == true

For an attribute, we just need to have a field. Requirements for an initializer are more strict. Especially taking https://github.com/dotnet/csharplang/blob/main/meetings/2022/LDM-2022-01-12.md#conclusion into consideration. #Closed

[Youssef1313]

_getMethod?.BodyShouldBeSynthesized == true ||
                    _setMethod?.BodyShouldBeSynthesized == true

@AlekseyTs Both of the logics don't seem to take semi auto properties into account, unless I'm missing something. For example, public string S { get => field; } should be allowed to have a field attribute target. But I agree your logic is a bit more accurate.

[AlekseyTs]

Done with review pass (commit 6)

[AlekseyTs]

LGTM (commit 7)

[AlekseyTs]

@Youssef1313 Thanks for the contribution.