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

Allow functions from an interface #426

Closed
Zastai opened this issue May 11, 2023 · 10 comments
Closed

Allow functions from an interface #426

Zastai opened this issue May 11, 2023 · 10 comments

Comments

@Zastai
Copy link

Zastai commented May 11, 2023

It is not an uncommon practice to use an interface as a container for public static methods.
However, adding @Function to a public static method in an interface results in a compilation failure:

[ERROR] Somehow this method got enclosed by something other than a class

This is a rather unhelpful message to being with: Which method are you talking about? Who is telling me this?

But as far as I can tell, PL/Java has no need to instantiate such an outer class at all, so there seems to be no real reason why it can't be an interface; that should not affect the ability to invoke a contained static method.

@jcflack
Copy link
Contributor

jcflack commented May 11, 2023

Allowing a static function on an interface seems reasonable on its face. The SQL/JRT standard does literally say

If R is an external Java routine, then the <Java method name> is the name of one or
more Java methods in the class specified by <Java class name> in the JAR specified by <jar name>. The
combination of <Java class name> and <Java method name> represent a fully qualified Java class name
and method name. The method name can reference a method of the class, or a method of a superclass of
the class.

and does not mention interfaces. Likewise, the PL/Java documentation has historically said "A Java function is declared with the name of a class and a static method on that class".

It could be fair to wonder whether the SQL committee really wrote class intending to exclude interfaces, or class in a more inclusive sense.

You might try having a static function on an interface, without the @Function annotation, and declaring it by issuing a suitable SQL CREATE FUNCTION directly. If that works, and there are no runtime issues, then it would seem the PL/Java runtime can already support the method being on an interface, and only the compile-time check in DDRProcessor would need to be relaxed to allow for an interface as well as a class.

The unhelpfulness of the error message is a bit disappointing. It is reported using the overload of Messager.printMessage that takes an Element argument, and is described in the Java API docs as producing a message "at the location of the element" in the source.

It might be worth checking what message is produced running javac standalone. I seem to remember noticing that Maven must be running the compiler programmatically and hooking the diagnostic messages, which it sometimes displays in less helpful ways than javac itself would.

@Zastai
Copy link
Author

Zastai commented May 16, 2023

Used mvn compile -X to get the command line it uses, and ran javac directly:

$ javac -d /xxx/target/classes -classpath ... -sourcepath /xxx/src/main/java: -s /xxx/target/generated-sources/annotations -g -nowarn -target 11 -source 11 -encoding UTF-8 /xxx/src/main/java/.../Hello.java
error: Somehow this method got enclosed by something other than a class
error: Somehow this method got enclosed by something other than a class
2 errors
$ javac -version
javac 11.0.18

Note also that that Hello source only has

public interface Hello {

    @Function(onNullInput = Function.OnNullInput.RETURNS_NULL)
    public static String hello(String toWhom) {
        return "Hello, " + toWhom + "!";
    }

}

but results in 2 separate diagnostics.

@jcflack
Copy link
Contributor

jcflack commented May 16, 2023

I am further disappointed then that even the standalone compiler is not displaying any of the information from the Element parameter passed to printMessage.

The duplication of the message probably just reflects the passes of operation within DDRProcessor. (It is possible the absence of Element information does also; I seem to remember learning that javac throws out its collected source locations at some stage of processing, and then printMessage cannot show that information if called later than that, and the API docs don't mention this.) Both issues might, therefore, be improved by reorganizing DDRProcessor to make this check earlier (I haven't looked).

But improving the message handling seems like a secondary priority. I am especially interested in whether you can compile Hello (without the @Function) and successfully use it in Postgres with a CREATE FUNCTION that you issue yourself.

If that works, then all that is needed is to relax this compile-time check to allow an interface also. If it does not work, then attention would also be needed to the runtime code.

@Zastai
Copy link
Author

Zastai commented May 16, 2023

I'll try to remember to try that tomorrow. I would expect it to work though (there is no difference in bytecode for such a call, it's always an invokestatic).

@jcflack
Copy link
Contributor

jcflack commented May 16, 2023

I'm thinking more about possible subtleties in the PL/Java code that looks it up and constructs the method handle for it. Not that I necessarily expect it won't work, but it's never been advertised or tested and there is room for complications.

@Zastai
Copy link
Author

Zastai commented May 17, 2023

Looks like it works, at least for the simple hello(toWhom) example case.

@jcflack
Copy link
Contributor

jcflack commented May 17, 2023

Good to know.

jcflack added a commit that referenced this issue Jun 6, 2023
The SQL/JRT standard has always just said class, but there is no
technical obstacle to using static methods on an interface, so
relax the annotation-processing-time check that was preventing that.

Addresses #426.
@jcflack
Copy link
Contributor

jcflack commented Jun 6, 2023

Would you have time to try a build from the feature/REL1_6_STABLE/issue426 branch?

The Appveyor tests appear failed, but I think that's just because Appveyor fell over.

@Zastai
Copy link
Author

Zastai commented Jun 7, 2023

Will do, although I currently don't know when that would be.

jcflack added a commit that referenced this issue Jun 13, 2023
PR #446, #443, #442, #441, #445, #444.

Addresses issues:

Bug #416 crash with SQL_ASCII database and bad vmoptions
Doc #419 better document the use of --add-modules in vmoptions
Bug #425 install_jar from http URLs, add test to CI
Feature #426 allow functions declared on an interface as well as a class
Track PG #434 postgres/postgres@b9b21ac broke unpackaged ALTER UPDATE
Track JDK #435 check and reject Java 20 builds with JDK-8309515 bug
@jcflack
Copy link
Contributor

jcflack commented Jun 13, 2023

Believed resolved in 1.6.5.

@jcflack jcflack closed this as completed Jun 13, 2023
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

No branches or pull requests

2 participants