Skip to content

Add thrown exceptions to MethodInfo (#633)#637

Merged
lukehutch merged 3 commits into
classgraph:latestfrom
moderneinc:thrown-exceptions
Jan 16, 2022
Merged

Add thrown exceptions to MethodInfo (#633)#637
lukehutch merged 3 commits into
classgraph:latestfrom
moderneinc:thrown-exceptions

Conversation

@jkschneider

Copy link
Copy Markdown
Contributor

No description provided.

}
}

public ClassInfoList getThrownExceptions() {

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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);

@lukehutch lukehutch Jan 14, 2022

Copy link
Copy Markdown
Member

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
Copy Markdown
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.

@jkschneider jkschneider Jan 14, 2022

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

2 participants