How to contribute on gatling-compiler?

[Pierre_Laporte1]

Hello folks

I would like to contribute a small fix to ZincCompiler.scala. Should I open a PR against branch 2.3 or against master?

Rationale:I am using Gatling with a dedicated plugin to benchmark Datastax Enterprise (our version of Apache Cassandra). There is currently an issue in the scala compiler that fails compilation whenever the DSE java-driver is used with Scala 2.11+ [1]. A work-around consists in adding a compiler option (-Ybreak-cycles). I added that option L49 in ZincCompiler.scala and can confirmed that it solved the issue. It is a one line patch, really.

Do you have any recommendation about that fix? Like, should I open a PR as-is or should I write something more complex so that it is only enabled when explicitly asked?

Thanks in advance for your guidance

Pierre

1: https://issues.scala-lang.org/browse/SI-3809

[slandelle]

Salut,

There are dozens of tricky options for scalac.
I didn’t know this one, and am not confident in hard coding it.
I mean, if scalac committers were 100% sure it’s harmless, why isn’t it enabled by default?
Before considering, I’d like to evaluate the risk that it could break existing code that would compile otherwise.

Also, did you check if there was a way to modify DSE java-driver code, just like it was done for Jsoup?
I couldn’t check for myself as this code is not open source.

It might be better to find a way to have those options configurable, with current ones as default, instead of hardcoded.

Cheers,

[slandelle]

Are you sure the code mentioned in this ticket still doesn’t compile with Scala 2.12.6 without this flag?
If not, you should probably chime in here as there’s a discussion for removing it from Scala 2.13.

[Pierre_Laporte1]

Hi Stéphane

Thanks for your feedback. I understand your concerns about hard coding that compiler flag. I think there should be a way through CommandLineOverrides. I will work on that today.

The DSE java-driver is currently under a major rewrite. As you suggests, in the next major release, the offending classes will be gone entirely. However that is not ready yet. And I don’t think we can introduce such changes on the current branch, given it could break many (if not all the) clients.

To be completely honest, I do not really understand why the scala compiler complains. The offending code is written in java and is already compiled. That whole issue is a mystery to me.

Cheers

Pierre

[slandelle]

Hi Pierre,

Thanks for your understanding.
Really, my concern is that there are probably less than 50 people in the world who understand what this flag actually does. And I’m not one of them. And I can’t find any reference if it’s brittle or safe.

If I get it right, the problem is with non static public inner classes being defined in a subtype and referenced in the parent one, eg: https://issues.scala-lang.org/browse/SI-3809?focusedCommentId=51099&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-51099.

You can also see this is Jsoup’s original code:

• Document extends Element

• Document has a public inner type OutputSettings that used to be non-static: https://github.com/jhy/jsoup/blob/9a2e19e8d3d24f79373f8dc41175b4d9ef0b79cd/src/main/java/org/jsoup/nodes/Document.java#L206

• Element implement some methods that take a Document.OutputSettings parameter: https://github.com/jhy/jsoup/blob/9a2e19e8d3d24f79373f8dc41175b4d9ef0b79cd/src/main/java/org/jsoup/nodes/Element.java#L969

• Those methods are defined (abstract) in Element’s parent Node: https://github.com/jhy/jsoup/blob/9a2e19e8d3d24f79373f8dc41175b4d9ef0b79cd/src/main/java/org/jsoup/nodes/Node.java#L378-L380
  I can’t tell if current DSE java-client uses the same kind of pattern.
  But such code sure is very ugly so I can understand why supporting this hasn’t been a top priority for the core scalac team, all the more as it seems pretty complicated to fix.

Is there any chance DSE java-client has such inner class that could/should be static so you could fix it the same way as Jsoup did?

[Pierre_Laporte1]

I opened a PR for a generic way of passing compiler flags instead of hardcoding -Ybreak-cycles => https://github.com/gatling/gatling/pull/3481

I am not sure we have exactly the same situation that jSoup. I looked at the code, we have a static inner class DseCluster.Builder that extends another static inner class Cluster.Builder. I could not find any explicit use of these in external top-level classes. Meanwhile, when I enabled -Ybreak-cycles, I can read this warning:

Breaking cycle in base class computation of class Cluster in com.datastax.driver.core (List(class Cluster, trait Closeable, trait AutoCloseable, class Object, class Any))

I am even more confused now. Cluster is a top level class. And that message suggests that, at most, it references itself?

Anyway, thanks for your guidance. If you want to investigate more, I am happy to continue this discussion. But at this point, I am not sure it is useful to spend more time on that. I can live with -Ybreak-cycles until our next major driver version is released. As soon as it hits beta, I will update the plugin to leverage the new API. If the problem persists, it will be fairly easy to change the API since no-one will depend on it yet.