Skip to content
This repository has been archived by the owner on Feb 23, 2023. It is now read-only.

Refine default access attribute defined on @TypeHint #1123

Closed
sdeleuze opened this issue Oct 1, 2021 · 4 comments
Closed

Refine default access attribute defined on @TypeHint #1123

sdeleuze opened this issue Oct 1, 2021 · 4 comments
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@sdeleuze
Copy link
Contributor

sdeleuze commented Oct 1, 2021

With @NativeHint(trigger = Foo.class, types = @TypeHint(types = EventListenerMethodProcessor.class, access = AccessBits.LOAD_AND_CONSTRUCT))

 {
    "name": "org.springframework.context.event.EventListenerMethodProcessor",
    "allDeclaredConstructors": true
  }

With @NativeHint(trigger = Foo.class, types = @TypeHint(types = EventListenerMethodProcessor.class))

 {
    "name": "org.springframework.context.event.EventListenerMethodProcessor"
  }

Given int access() default AccessBits.LOAD_AND_CONSTRUCT; it should be the same but the code in org.springframework.nativex.type.Type#unpackTypeHint seems to work differently when the attribute is defined or not.

@sdeleuze sdeleuze added the type: bug A general bug label Oct 1, 2021
@sdeleuze sdeleuze added this to the 0.11.0-M1 milestone Oct 1, 2021
@aclement
Copy link
Contributor

aclement commented Oct 3, 2021

This is not a mistake but was it some lingering design we should tidy up. Originally people had no real clue about what to set for visibility on their types and there were lots of different kinds to consider (configurations, listeners, processors, selectors, registrars, regular types, array types) - so we never default back to the default access value on the TypeHint annotation. Instead we used Type#inferAccessRequired(Type) if they didn't provide a value. This enabled us to be smarter (for example, adding more than CLASS access for an array type is meaningless so the default isn't quite right in that case anyway - for different kinds of types the default is really different - annotation types, enum types, array types, etc).

With Aot we have simplified inferAccessRequired and it doesn't get called for a variety of Springy things it used to. I just experimented with letting it default and ran all the samples. I was still concerned that there might be hints still relying on the smarts, rather than the defaulting. And, yes, around 25 samples start failing if you just let it default.

I think we should fix up the hints to be correct, even perhaps not have a default, it's easier now there are fewer and they aren't full of Spring stuff. However, up to you whether we want to tackle that before/after M1. It means revisiting all the failing samples and working out what is wrong with the hints. I added some simple diagnostics that can come up with what is being inferred differently to the default, it tells me 1762 of the types referred to in type hints across all native hints are smartly inferred to be something different to the default - that doesn't mean the inferred thing is always 100% correct of course, it just means it is much more compatible (it may still be overkill and less access is possible). I'm tempted to get M1 out on our somewhat stable setup then tackle things like this.

@aclement
Copy link
Contributor

aclement commented Oct 3, 2021

This commit aclement@f6b035e turns on diagnostics and lets them default. (Have to search for MISMATCH in the target/native/output.txt)

@sdeleuze sdeleuze modified the milestones: 0.11.0-M1, 0.11.x Oct 4, 2021
@sdeleuze
Copy link
Contributor Author

sdeleuze commented Oct 4, 2021

Ok let's tackle that post M1.

@sdeleuze sdeleuze modified the milestones: 0.11.x, 0.11.0-M2 Oct 4, 2021
@sdeleuze sdeleuze changed the title Wrong reflect-config.json generated with no access attribute defined on @TypeHint Refine default access attribute defined on @TypeHint Oct 26, 2021
@sdeleuze sdeleuze self-assigned this Oct 26, 2021
@sdeleuze sdeleuze added type: task A general task and removed type: bug A general bug labels Oct 29, 2021
@sdeleuze sdeleuze modified the milestones: 0.11.0-M2, 0.11.0, 0.11.0-RC1 Nov 1, 2021
@sdeleuze
Copy link
Contributor Author

Working on it, should have something shortly.

@sdeleuze sdeleuze added type: enhancement A general enhancement and removed type: task A general task labels Nov 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: enhancement A general enhancement
Development

No branches or pull requests

2 participants