-
Notifications
You must be signed in to change notification settings - Fork 903
polish the TypeInfo system
#2201
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
base: master
Are you sure you want to change the base?
Conversation
it should be replaced by a more general solution like `TypeInfo.visitComponents`
Using this factory to resolve self-referencing type variables (`T extends Enum<T>`) will cause inf loop
- support formatting VariableTypeInfo to `K extends SingleBound` - cleaner overall design
rhino/src/main/java/org/mozilla/javascript/lc/type/TypeFormatContext.java
Outdated
Show resolved
Hide resolved
aardvark179
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is okay, but if you are going to do a big stack of commits as a PR with the expectation that it's review as that stack of commits then it's helpful to do spotless as you go, and apply any cleanup as a fix up commit. That way people won't stack up a bunch of comments on earlier commits that are then fixed up later—GH doesn't help hear by highlighting that those lines don't make it to the end.
| ignoring from: "org.mozilla.javascript.lc.type.TypeInfo", to: "org.mozilla.javascript.lc.type.impl.*" | ||
| ignoring from: "org.mozilla.javascript.lc.type.TypeFormatContext", to: "org.mozilla.javascript.lc.type.impl.ClassSignatureFormatContext" | ||
|
|
||
| ignoring from: "org.mozilla.javascript.lc.type.*", to: "org.mozilla.javascript.lc.type.impl.*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth taking a look at the overall recycle rules for our base package lc.type as we keep having to fix them during development. If the intention is that they reflect the future separation of the lc components from the main Rhino module then lc should have access to whatever is exported from the main module, and we really shouldn't care about anything more precise than that.
Current
TypeInfosystem works good enough in common use cases, but there still some rough corners / lack of design / overengineering that need to be polished.All changes are documented in commit messages. Notable changes are:
TypeInfoFactory.NO_CACHEis removed, because it's unable to handle self referencing type variables, e.g.E extends Enum<E>Outer<T1>.Innerstyle parameterized type, including correct formatting and consolidation mapping generation.TypeFormatContextis redesigned to be more consistent and easy to use.