JetBrains / java-annotations

Annotations for JVM-based languages.
Apache License 2.0
404 stars 47 forks source link

Contract solution for a method always returning the same for same arguments #15

Closed burdoto closed 5 years ago

burdoto commented 5 years ago

I'd love to have a way of defining with a Contract that a method always returns the same instance of the same parameters, something like this:

@Contract(equalResults = true)
public FileConfiguration getConfig(String configName) {
    //...

The inspiration for this was this discussion: https://github.com/burdoto/e2uClaim/pull/1#discussion_r281180517

This contract should then result in a warning that informs the user that repetitively calling the method for same arguments is unnecessary and the result can be stored in a local variable.

This is useful because like in this special case, a #getConfig(String) method may always return the same configuration for the same string parameter, while e. G. InputStream#read(int) does not always return the same on equal calls. Now, I know this example is a bit niche, so it might even be best to assume @Contract(equalResults = true) for most methods. Let's discuss this idea.

amaembo commented 5 years ago

Hello!

We already have @Contract(pure = true) which tells the analyzer that the method call doesn't change the visible state of the application. This also assumes that if you call such a method several times consequently with the same arguments, you'll have the same result. So seems that pure supersedes the suggested equalResult and particularly in your case the new annotation is unnecessary.

Still, for static analyzers it's not enough to annotate only getConfig somehow. Analyzer must also know that any other methods called in-between do not change the data used by the getConfig. E.g. in your case it's quite possible that the set method called after getConfig actually updates the file read by getConfig.

So I don't see a good new annotation which would help in your case.

burdoto commented 5 years ago

Thanks for the clarification, I wasn't aware about this behavior of @Contract(pure = true).

Do you think the analyzer works better if I were to make getConfig non-static?

amaembo commented 5 years ago

Do you think the analyzer works better if I were to make getConfig non-static?

If you are speaking about the IntelliJ IDEA analyzer, as far as I know, it doesn't detect excessive calls like this. It's quite difficult to make such warnings useful and not annoying at the same time. E.g. often a method like getConfig just returns a field or uses a lazy initialization technique like double-checked locking which is also cheap, so performance handicap for calling such a method in the loop is small and doesn't outweigh the additional code complexity. In your case, it seems that getConfig reads the file every time which is surely expensive, but it's difficult for the analyzer to differentiate expensive methods from non-expensive.

burdoto commented 5 years ago

Oh wow. You seem to have spotted a mistake that I seem to have made, my case should only load if not loaded yet. 🥴

Anyways, thanks for the clarification!