Consider minTargetVersion as always supported

[arixmkii]

Fixes #13810

[test_java8]

Allows to use consistent scalac options running with JDK 1.8 and higher if targeting Java8 bytecode level.

Don't know if there are more places to check for more code changes required.

[arixmkii]

CLA signed

[prolativ]

I'm wondering whether this simple solution might break something. Basically when -release is specified we rely on some mechanisms specific to JDK 9+ (compare #10746). I might inverstigate further into that but probably the safest way not to break anything would be to just ignore -release 8 at the stage of reading settings when on JDK 8

[arixmkii]

I'm wondering whether this simple solution might break something.

Same here. Such a simple change looked very suspicious to me either, but I don't know the codebase, where to check for the JDK specific processing.

I might inverstigate further into that but probably the safest way not to break anything would be to just ignore -release 8 at the stage of reading settings when on JDK 8

Or apply it the way -Xrelease 8 works when run with JDK 8 (probably a no-op, when running with JDK 8).

Main point of the original issue and this PR specifically is to make it looking consistent for the client and left the JDK internals magic to the compiler, not exposing it to every build.

[prolativ]

You could try writing some test, which compiles and runs some code with -release 8 option, and then force the CI to test it with JDK 8 (I've never tried that but probably adding [test_java8] in the PR's description should work)

[arixmkii]

Will take a look on how to write a test later this week.

@prolativ could you suggest a good one I can use as a starting point to understand how tests are expected to look?

[prolativ]

Here's a general description of how tests in scala 3 repo work https://dotty.epfl.ch/docs/contributing/testing.html

In your particular case you should have a look at other tests in tests/run-custom-args directory as examples and see how compiler options are defined for them in CompilationTests.scala

[arixmkii]

Added test, which checks the consistent error compiling JDK 9 application with -release:8 on supported JDKs (same error output for JDK 8 and JDK > 8).

Can't imagine if it is possible to compose a test, which will compile JDK 9 API enabled code running on JDK 8 (should be possible on unaltered JDK only if it is possible to specify explicit JDK classpath while running compiler on JDK 8, but if someone will do so, they should account for possible classpath related errors on their own and should not use -release:8 key in the first place).

[prolativ]

I did some analysis and it looks like the cases I was afraid would fail with -release 8 on JDK 8 should actually be handled correctly (actually I found one suspicious behaviour: compiling with -release X on JDK X and without the flag on the same JDK seem to have slightly different behaviours - both for X=8 and higher - more specifically FileZipArchive#openZipFile will return an instance of JarFile in the former case and a more general ZipFile in the latter. Not sure if this is totally OK or we should unify this somehow - @smarter any ideas?)

Additionally I think we should also have some positive test to check that -release 8 actually does work on JDK 8. So this time you'll need to similarly add a test in run-custom-args with some simple code to make sure it compiles and doesn't crash at runtime. And as I wrote before - we should force your tests to run in the CI. Try modifying the description of your PR and add [test_java8] tag in there (probably best in a new line) - I hope this should work

[smarter]

Not sure if this is totally OK or we should unify this somehow

I don't think it matters much either way really, though I think it'd be fine to make -release X do nothing at all on JDK X.

[prolativ]

@smarter There's one more thing that I realized now - if one does not specify -release then the bytecode is by default generated for JDK 8 instead of the currently used JDK. So making release X a noop for every JDK X would change the behaviour of the compiler in a way which is not just a simple bugfix because people may rely on the fact that they get bytecode for JDK 8 by default

[smarter]

people may rely on the fact that they get bytecode for JDK 8 by default

It's a dangerous thing to rely on since if they have mixed java/scala code, the javac compiler will emit JDK X bytecode by default, so I think we should change that behavior at some point, but only in a minor release.

[prolativ]

Ok, if we decide to change that, scala 3.2 seems to be the most appropriate point as we'll be adding -scala-target at the same time.
BTW our current behaviour at least seems to be consistent with what scala 2.13 does

[arixmkii]

Added pos test with basic app using JDK 8 level APIs.

[prolativ]

@arixmkii The CI doesn't run for your PR. I guess it's because you need to resolve the conflict with the master branch first.

[arixmkii]

@prolativ will fix them. What is the preferred way in this project - rebasing or merge from master to the feature branch?

[prolativ]

Personally I prefer rebasing (with squashing) because it gives you nicer history but honestly speaking this doesn't seem to be enforced anyhow in the repo

[arixmkii]

Rebased to latest master.

[prolativ]

The CI failed but this seems to be a problem on master, not this branch

[arixmkii]

Saw the same failure locally after rebasing - both with JDK 8 and JDK 11. Did clean checkout of dotty and it was ok (single test run). Running sbt clean and then new sbt shell for tests fixed it magically for me, so, I decided to push the rebased version. But something is unstable in master.

[prolativ]

Yeah, we'll need to wait until #13878 (comment) gets solved to merge this

[prolativ]

@arixmkii try to rebase to the newest master and resolve any conflicts. I hope this should work now

[arixmkii]

Rebased to latest master.