Eliminate templated STRING_TYPE and use a factory

[headius]

Currently there are several templates that must be customized to either use java.lang.String or org.jruby.RubySymbol for various string or identifier representations. This is done via the JAVA_STRING_TYPE constant which is customized in template.rb to the specific types used by TruffleRuby and JRuby. Despite the name, this appears to only be used for representing identifiers, e.g. for variables, methods, and class names.

I'd like to eliminate this variability in the Java API by instead providing a factory class that can construct appropriate representations.

The interface might look something like this:

public interface IdentitierFactory<IDType> {
    public IDType newIdentifier();
}

The parser would be set up to accept such a factory on instantiation, and indentifiable nodes it produces would aggregate values of T rather than the templated String or RubySymbol that they do now. This would not change usage of the API significantly, other than requiring TruffleRuby and JRuby to both pass in a factory object for creating identifiers.

Nodes that aggregate identifiers (or aggregate nodes that aggregate identifiers) would be generified to propagate this IDType parameter.

public class Nodes {
    ...
    public static class LocalVariableReadNode<IDType> extends Node {
         ...
         public final IDType name;

This would allow a single Java API to be published that all JVM consumers of the Prism API could use, with only the backends differing (initially publishing a WASM version and later a multi-platform native backend).

As this will require some discussion I'm putting it here as an issue and will for now proceed with publishing a JRuby-specific API artifact so we can ship that in JRuby 10.1.

[eregon]

What are concrete issues with the current templated approach?
I don't see a clear problem/motivation in the issue description.

My concerns with this proposal are:

• Slower performance due to extra checkcast on every usage of a generic type
• More verbose node definitions, making it look needlessly complicated and boilerplate-Java-like
• More verbose creation of nodes (at least TruffleRuby uses that to desugar some nodes into simpler ones)

IOW, this sounds a perfect use case for templating given we already have templating.

[eregon]

What about always using java.lang.String as the JAVA_STRING_TYPE?
From your messages on Slack I thought that would be the proposal.
That's a neutral type, makes the API significantly easier to use than a factory, and should work fine under JRuby too.

I agree it doesn't make much sense to publish under org.ruby_lang if it's still JRuby-specific (would be better under a JRuby Maven namespace then).

[headius]

I had a separate discussion with @eregon to clarify some of this and wanted to summarize here.

• JRuby cannot (or would rather not) move to String for identifiers.

We need to represent all identifiers as original bytes + encoding, because this does matter for metaprogramming when literal symbols and identifier symbols don't match. We had bug reports from real users prior to making that move, including 3rd-party APIs that failed due to the mismatch.

• Other string representations in the Prism Java AST use byte[] + source encoding, so I believe that may be the best common path forward.

This would be the simplest option overall, I believe. JRuby would be able to consume the byte[] directly (without copying), preserving the original characters. It's a bit less convenient for TR, which will still need to turn them into java.lang.String, but @eregon says "it'd work and be fairly neutral".

Another point in its favor is that the deserialization process will end up creating byte[] anyway, so if we just have it stop there it's not really any additional overhead.

• My goal with this Maven artifact is to produce an official Java API that JVM-based consumers can use for Ruby tooling.

Think things like JetBrains's RubyMine, which has been notoriously bad about keeping up with language changes. It is a backburnered project at this point, but if we could produce Prism as an official API they might make the move. There are other potential consumers out there as well.

Publishing this as an official API means it should live under the org.ruby-lang namespace.

[headius]

Also, a note on versioning Prism versus versioning the JVM artifacts...

Because the parser artifact for JRuby (JVM WASM or native wrapper with location info removed) would be a self-contained release, with no external dependency on the Prism gem or this repository, I think we can version it however we like. We can also do official releases of that artifact on-demand (for now, at least). That frees both sides of the project to handle versioning however they like and release on independent schedules.

The parser artifact for the gem (JVM WASM-based library with full location info) would be locked to the Prism version and released along with the gem at the same time, but since it's hidden within the gem's API there's less concern about any exposure to JVM users.

The bits and pieces are falling into place in my head, but I think we have a path forward.

[eregon]

One more thought on this is for Loader#loadConstant:

prism/templates/java/org/ruby_lang/prism/Loader.java.erb

Lines 216 to 218 in aa6bce6

	private <%= string_type %> loadConstant() {
	return constantPool.get(buffer, loadVarUInt());
	}

We might want, in TruffleRuby at least, to return just the constant index or maybe the constant index + a byte[] wrapped in some object.
The idea there is it seems needlessly inefficient to do hashmap lookups to find the frame offset from the local variable name (multiple times for the same variable, once per usage), when we could just have a constant index->frame offset mapping as an int[] (one int[] by scope) by looking the names only once.

If we go for returning just the constant index, that would still be neutral/universal and we'd then need a way to make it easier to use the constant pool with custom conversion or to build your own (currently it's a private class, doesn't expose the size of it, etc).
It would be less convenient than Strings or byte[] for general Java usage, but maybe not by much.
I think general Java/IDE usage java.lang.String would still be the best (as it is currently).

Because this is not figured out yet, if you want to make progress for this I would suggest to support JAVA_STRING_TYPE to be byte[] and it could even be the default. And we'd keep j.l.String for TruffleRuby for now, to avoid a probably useless migration to byte[] when we'd actually want constant indexes instead for TruffleRuby.

I'll try to experiment with returning just the constant index when I have some time.

BTW probably we should rename JAVA_STRING_TYPE to JAVA_IDENTIFIER_TYPE (as you said).
Could also be JAVA_CONSTANT_TYPE since that's the Prism concept for that, but might be rather unclear.

[eregon]

Also, a note on versioning Prism versus versioning the JVM artifacts...

Because the parser artifact for JRuby (JVM WASM or native wrapper with location info removed) would be a self-contained release, with no external dependency on the Prism gem or this repository, I think we can version it however we like. We can also do official releases of that artifact on-demand (for now, at least). That frees both sides of the project to handle versioning however they like and release on independent schedules.

It would still be very useful to know which Prism version is in which jar though, without consulting a manually-maintained mapping for it. The easiest would be to use the same version, or maybe with an extra component or so.

Probably a good idea to do some 0.x releases though until things are more fledged out and it's actually ready for general Java usage.

[headius]

The easiest would be to use the same version, or maybe with an extra component or so.

Yeah, something like we've been doing for our ActiveRecord adapters (two-digit Rails version followed by other components) might make sense, if that fits Prism's versioning.

I think we can deal with that when we're ready to release a version > 0.x for more general usage.

[kddnewton]

@headius whatever y'all agree upon is fine with me

[headius]

@kddnewton I will finalize the maven PR today so that can get merged and do a separate PR to switch the identifier type. Then release number 0.0.1!