Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle private properties in the compiled model #33409

Merged
merged 1 commit into from
Apr 4, 2024
Merged

Conversation

AndriySvyryd
Copy link
Member

Add full support for service properties
Use a consistent pattern for unsafe accessors
Store the unsafe accessor names in an annotation
Refactor the different mechanisms for tracking variables in the generated code into a single bidirectional dictionary

Fixes #29761
Fixes #24904
Fixes #24900

public static string GetUnsafeAccessorName(MemberInfo member)
{
StringBuilder stringBuilder = new();
stringBuilder.Clear().Append("UnsafeAccessor_");
Copy link
Member

Choose a reason for hiding this comment

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

Is this our naming convention, or a convention recommended from somewhere else?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think there's a specific recommended naming convention for unsafe accessor (see API docs with samples), this is also what I used on the query side when generating these.

No strong feelings of course (this is all "internal" generated code), it just seemed like a code idea to identify what these methods are etc.

Copy link
Member

Choose a reason for hiding this comment

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

I just noticed that it generates very long names. Not a big deal, but given the size of generated code has been an issue it was something that occurred to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just made it consistent with what query generates. The idea is to have a unique name that still means something. If Shay agrees we can just use the member name + uniquefier to keep it short.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, am fine with whatever - remember this is just generated code so nothing's very important here (except for uniqueness).

{
if (assignmentKind is not SyntaxKind.SimpleAssignmentExpression)
{
throw new NotSupportedException("Compound assignment not supported");
Copy link
Member

Choose a reason for hiding this comment

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

Hard-coded string?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is from Shay's PR, I was reading this as "not implemented", we could handle compound assignments by calling the getter and then the setter

Add full support for service properties
Use a consistent pattern for unsafe accessors
Store the unsafe accessor names in an annotation
Refactor the different mechanisms for tracking variables in the generated code into a single bidirectional dictionary

Fixes #29761
Fixes #24904
Fixes #24900
@AndriySvyryd AndriySvyryd merged commit 7213003 into main Apr 4, 2024
7 checks passed
@AndriySvyryd AndriySvyryd deleted the Issue29761 branch April 4, 2024 05:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants