Conversation
| } | ||
| } | ||
|
|
||
| private static class JDKStubxLibraryModels implements LibraryModels { |
There was a problem hiding this comment.
We shouldn't need this code at all. Just refactor the current ExternalStubxLibraryModels to load the JDK astubx file. You will have to move the JarInferEnabled check inside the constructor maybe. But we don't want to copy all this code.
|
This PR (which needs some work still) will address a good deal of #950. But, it's also going to potentially cause a lot more errors to be reported in existing code. I'm wondering about our release strategy for including this. One option would be to only load these models in JSpecify mode, and give an option to turn off the new models, in case someone wants to upgrade but wants to address the new errors one module at a time. Alternately, we could do a minor release before this lands, and then make this part of a "major" release, to make clear there will be some work to upgrade, but not have an option to disable. Or maybe we should do both ("major" release and include an option to disable)? @sdeleuze curious for your thoughts here. You can see here some of the new warnings NullAway will emit on Spring Framework once this lands. |
| while (methodNameAndSignature.contains(" ") | ||
| && methodNameAndSignature.indexOf(" ") < methodNameAndSignature.indexOf("(")) { | ||
| methodNameAndSignature = | ||
| methodNameAndSignature.substring(methodNameAndSignature.indexOf(" ") + 1); | ||
| } |
There was a problem hiding this comment.
Why do we need this change?
There was a problem hiding this comment.
It's because of methods MyClass<? super T> methodName() like this. Then, making the substring using the index of space will result in super T> methodName().
There was a problem hiding this comment.
Ok. Can we move this change to a separate PR then, and add a unit test for it?
There was a problem hiding this comment.
Yes, I can do that.
|
I will provide a feedback shortly. |
|
Sorry for the delay. So that's a very valuable change but also a quite disruptive one. I am not sure it is really tied to JSpecify even if using both JSpecify and astubx make sense, so I would not link the 2 too much. I think I would suggest to introduce it disabled by default with an option to enable it in a "minor". |
|
@haewiful will recreate this one based on the latest code |
This pull request adds some nullness check support for modules in the JDK package. It adds the
astubxfile that contains the nullness information for the modules in the JDK package and loads it to NullAway.