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

Corrected class name validation to no longer fail for Kotlin classes on class path containing special characters #1884

Merged
merged 5 commits into from Dec 22, 2021

Conversation

studro
Copy link
Contributor

@studro studro commented Dec 19, 2021

Closes #1883.


Make sure these boxes are checked before submitting your PR -- thank you!

  • Added an entry into CHANGELOG.md if you have changed SpotBugs code

ThrawnCA
ThrawnCA previously approved these changes Dec 19, 2021
Copy link
Member

@KengoTODA KengoTODA left a comment

Choose a reason for hiding this comment

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

Just one mandatory request exists in isValidBinaryClassName() methods, to follow the definition in the JVMS.
Other feedback are just my 2 cents. Thanks for your PR!

Comment on lines 197 to 200
return className.startsWith("[") &&
(isValidArrayFieldDescriptor(className.substring(1))) ||
isValidClassFieldDescriptor(className.substring(1)) ||
isValidBaseTypeFieldDescriptor(className.substring(1));
Copy link
Member

Choose a reason for hiding this comment

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

It's just my personal and unreasonable motivation. JIT could be enough.

Suggested change
return className.startsWith("[") &&
(isValidArrayFieldDescriptor(className.substring(1))) ||
isValidClassFieldDescriptor(className.substring(1)) ||
isValidBaseTypeFieldDescriptor(className.substring(1));
String tail = className.substring(1);
return className.startsWith("[") &&
(isValidArrayFieldDescriptor(tail)) ||
isValidClassFieldDescriptor(tail) ||
isValidBaseTypeFieldDescriptor(tail);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually thinks this improves readability too, so this might be a good change. 👍

However, I am concerned about the possibility of throwing an IndexOutOfBoundsException if this is called with an empty string (instead of returning false). I have added an isEmpty() check in the caller and documentation to mitigate this concern.

Comment on lines 178 to 181
return isValidBinaryClassName(className) ||
isValidDottedClassName(className) ||
isValidArrayFieldDescriptor(className) ||
isValidClassFieldDescriptor(className);
Copy link
Member

Choose a reason for hiding this comment

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

👍 Confirmed that all descriptions listed in the JVMS are covered.

@studro
Copy link
Contributor Author

studro commented Dec 20, 2021

Just one mandatory request exists in isValidBinaryClassName() methods, to follow the definition in the JVMS. Other feedback are just my 2 cents. Thanks for your PR!

Thanks for your quick and thorough review. I've attempted to address the comments you've raised and will be happy to make any further changes as necessary. 😄

KengoTODA
KengoTODA previously approved these changes Dec 21, 2021
Also added logic to allow existing cases where a field descriptor
or dotted class name is provided instead of a slashed (binary)
class name.
* Documented private methods in class name validity checker with links to JVM spec.
* Cleaned up array type checking code.
* Added test to ensure that empty class names are rejected.
@studro
Copy link
Contributor Author

studro commented Dec 21, 2021

I am very sorry to be difficult, but one final review is required, if possible.
I've updated the exception documentation as I noted in #1884 (comment). While this was only a minor issue, what I had written was completely wrong.
I also noticed that I had the incorrect commit author email set for this project (my personal instead of no-reply github email), so I've updated history with a force push to correct that.
Thanks for your patience and hopefully this is now the end of this issue! 😊

@KengoTODA KengoTODA merged commit 1345756 into spotbugs:master Dec 22, 2021
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.

Mixed Kotlin/Java projects can produce classes that crash the tool due to strange class names
3 participants