ReflectionHints
with fields
on default adds allowWrite
while in graal its value is optional and should be set to true
if write should be allowed. We should not write this field unless user explicitely defines it so that we can have whatever graal does when field is not present.
{
"name": "org.example.Clazz",
"fields": [
{
"name": "value",
"allowWrite": true
}
]
}
Though looking graal sources these fields allowWrite
and allowUnsafeAccess
has been deprecated since 21.1
meaning we could probably just remove both.
https://github.com/oracle/graal/blob/22a947877c1d1cff07186999d5b9365f09c2466f/sdk/src/org.graalvm.nativeimage/src/org/graalvm/nativeimage/hosted/RuntimeReflection.java#L120-L129
Comment From: sreenath-tm
I'm interested in contributing to this issue, so before I start working it, would you mind sparing your time pointing me to some resources to get started.
Comment From: snicoll
The issue is yours @sreenath-tm but please keep in mind that our next milestone is due in 10 days. If you don't get to it by then, just let us know and we'll take over.
The changes are to be done by removing FieldHint
altogether and changing TypeHint.Builder
to hold a Set
of field names, to be translated to MemberHint
in TypeHint
's constructor. Further simplifications are due in the predicates but we can polish that as part of merging. Let me know if you have any question.
Comment From: sreenath-tm
The MemberHint does not have a Builder so for replacing the removed FieldHint where can we generate the set of field names that is referenced in the TypeHint class.
Comment From: snicoll
In TypeHint.Builder
there is a private final Map<String, FieldHint.Builder> fields = new HashMap<>();
. I am suggessting to replace that with a Set<String>
. If the field is registered multiple times, the set will make sure to keep only one copy. MemberHint
should no longer be abstract or we can maybe keep a FieldHint
. Does that help?
Comment From: sreenath-tm
1 ) Is moving the code from our FieldHint to MemberHint recommended.
2 ) I replaced the FieldHint related references and replaced with Setpublic Builder withField(String name, FieldMode mode) {
return withField(name, FieldHint.builtWith(mode));
}
and public Builder withField(String name, Consumer<String> fieldHint) {
FieldHint.Builder builder = this.fields.computeIfAbsent(name, FieldHint.Builder::new);
fieldHint.accept(builder);
return this;
}
Comment From: snicoll
- No. FieldHint should only have a
name
attribute, which is what this issue is all about. - They should go away completely and replaced by
withField(String name)
as the additional attributes should be removed.
Comment From: snicoll
@sreenath-tm how is it going? Sorry to say but we'd like this (and a few related changes) to get in earlier next week so that the team can adapt their code in preparation of the release Thursday. If you have some work in progress you'd like to share, you can submit a PR and I can complete it if necessary. Thanks!
Comment From: sreenath-tm
@snicoll I have changed the corresponding FieldHint and all the instances of FieldHint. I am still figuring out the usages of the FieldHint class and the impact the removal it can cause.Will raise a draft PR so that you can also take a look at it.
Comment From: snicoll
Closing in favor of PR #29130