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

Add thrown exceptions to MethodInfo (#633) #637

Merged
merged 3 commits into from
Jan 16, 2022

Conversation

jkschneider
Copy link
Contributor

No description provided.

@@ -318,6 +324,24 @@ public String getTypeSignatureOrTypeDescriptorStr() {
}
}

public ClassInfoList getThrownExceptions() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When a thrown exception is a generic type variable, the Exceptions attribute refers to the bound type, which I think makes sense to return here?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think so, or if all else fails and the type variable cannot be resolved or does not have a supertype (e.g. E extends IOException), just return Exception as the concrete type, I think.

It's a shame Java doesn't have union types, since it would be nice to return TypeVariable or ClassInfo depending on whether or not the class variable is unresolved.

Also you can discard type parameters like T in MyException<T>. If anyone needs those, they can use the existing method to get the generic throws type signature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also you can discard type parameters like T in MyException

I believe that is how this functions currently.

thrownExceptions = new ClassInfoList(thrownExceptionNames.length);
for (String thrownExceptionName : thrownExceptionNames) {
ClassInfo classInfo = scanResult.getClassInfo(thrownExceptionName);
thrownExceptions.add(classInfo);
Copy link
Member

@lukehutch lukehutch Jan 14, 2022

Choose a reason for hiding this comment

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

classInfo may be null here -- is it helpful to have a null placeholder in the list for classes that could not be found (because they were not discovered during the scan)?

Actually I forgot one more change you should make -- look at extendScanningUpwards() -- you should schedule the thrown exceptions for scanning in that method. If you make this change, the chance of classInfo being null is lower, because any named exceptions will be scanned as "external classes" even if they are not in the accept list.

That raises the question though, is the Exception attribute always present, even if there is a generic throws type signature present?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That raises the question though, is the Exception attribute always present, even if there is a generic throws type signature present?

Yes it is. The Exception attribute will return the bound type though. So it just kind of works as is without having to strip parameterizations and such off of throws.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is it helpful to have a null placeholder in the list for classes

Null values removed in the latest commit.

@lukehutch
Copy link
Member

Looks good to me. Thanks for all your work on this! Will merge now.

@lukehutch lukehutch merged commit 1e7c692 into classgraph:latest Jan 16, 2022
@lukehutch
Copy link
Member

I'm working on another bugfix, which might take a couple of days to sort out, but I'll push this out in a new version as soon as I can. Thanks for your patience, I'm swamped right now!

@jkschneider
Copy link
Contributor Author

Thanks for your patience, I'm swamped right now!

No worries, and thanks for your incredible responsiveness.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants