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

Trying to support changing member accessibility #108350

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

thaystg
Copy link
Member

@thaystg thaystg commented Sep 27, 2024

Fixes #108097

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-EnC-mono Hot Reload for WebAssembly, iOS/Android, etc label Sep 27, 2024
@thaystg
Copy link
Member Author

thaystg commented Sep 27, 2024

@lambdageek if this is what you were suggesting I will create a test case on wasm debugger tests after your review.

@thaystg
Copy link
Member Author

thaystg commented Sep 27, 2024

@lambdageek it also works if I don't call mono_field_resolve_type

}
} else {
Copy link
Member Author

Choose a reason for hiding this comment

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

@lambdageek It also works if I completely remove this else.

Copy link
Member

Choose a reason for hiding this comment

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

@thaystg in your test was the field previously used? if field->type != NULL then mono_field_resolve_type should overwrite it and include the new visibility attributes.

Something like the f2 function in this example #108097 (comment) - should see the new visibility. So if you're changing public->private then f1 that already ran before the update will keep working, but f2 created after the update should throw a MemberAccessException

Comment on lines +1902 to +1903
if (!is_addition && token_table == MONO_TABLE_FIELD) {
add_member_typedef = log_token;
Copy link
Member

Choose a reason for hiding this comment

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

Let's not do it like this. the add_member_XXX variables are there because the ADD functions all come paired with another row with a DEFAULT function - it's kind of a hack to make sure we process them together. we don't need it for rows that are self-contained.

Just move the assert in the MONO_TABLE_FIELD case into the is_addition branch:

  if (is_addition) {
     g_assert (add_member_typedef);
    // the rest is all the same
  } else {
     // just use log_token instead of add_member_typedef
 }

@lambdageek
Copy link
Member

@thaystg FYI, from Tomas:

All members and types.

We also need to support changing visibility for methods/properties/events/nested types

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-EnC-mono Hot Reload for WebAssembly, iOS/Android, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EnC: Mono throws when member accessibility is changed
2 participants