Skip to content

Java: Add Callable.getErasureStringSignature() #8761

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Marcono1234
Copy link
Contributor

I assume the intention behind Callable.getStringSignature() is to provide a concise representation of the signature. However, there are two problems with the representation of parameter types:

  • It depends on Type.toString() which has no guaranteed format
  • For parameterized types, it includes all type arguments
    • This makes usage quite verbose, e.g. doSomething(Function<? super Integer,? extends String>)
    • The type arguments are separated only by a comma, but not by a space (unlike the callable parameter types)

This defeats the purpose of getStringSignature() a bit. This could be solved by adjusting the getStringSignature() implementation, but I chose to instead add a separate predicate called getErasureStringSignature().

Open questions:

  • Should the existing getStringSignature(), hasStringSignature(string) and paramsString() be deprecated?
  • With the new getErasureStringSignature(), might the erasure for type variables be too error-prone? E.g. you have to know how the erasure is determined and check the bounds for the variable. On the other hand, the question is to what extend the name of a type variable can be considered an implementation detail (which might change between versions).

Any feedback is appreciated!

@Marcono1234 Marcono1234 requested a review from a team as a code owner April 16, 2022 20:08
@github-actions github-actions bot added the Java label Apr 16, 2022
string paramsString() {
exists(int n | n = this.getNumberOfParameters() |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have rewritten this to use concat. The public Git history did not indicate why this was previously implemented this way. So if there are any internal reasons for this, I can revert the change if you want.

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

Successfully merging this pull request may close these issues.

1 participant