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

Support for Scala 3.4.0 #1394

Closed
wants to merge 5 commits into from

Conversation

Gedochao
Copy link
Contributor

No description provided.

@Gedochao Gedochao marked this pull request as draft February 15, 2024 09:38
@Gedochao Gedochao force-pushed the bump-scala-3.3.2-3.4.0 branch 4 times, most recently from f9e5c46 to 9133cc0 Compare February 15, 2024 13:27
@Gedochao
Copy link
Contributor Author

Loading Scala 3 binary from /Users/pchabelski/IdeaProjects/Ammonite2/out/amm/interp/api/3.4.0/compile.dest/classes/ammonite/$ivy/$.class. It should have been loaded from `.tasty` file. This `.tasty` file is missing. Try cleaning the project to fix this issue.

This seems 3.4.0-specific and pops up all over the place... 😕
Replicable for example with ammonite.integration.BasicTests.testIvySnapshotNoCache (and some other tests)

@Gedochao
Copy link
Contributor Author

Gedochao commented Feb 16, 2024

After some investigation, the failed tests for 3.4.0 seem to be caused by this:
scala/scala3#19702
As it seems identifiers containing $ will not be allowed long term, it may require fixes on Ammonite's side.

@Gedochao
Copy link
Contributor Author

I don't know the codebase enough to attempt this, but I suppose we'd need to refactor Ammonite to get rid of identifiers containing the $ special character.
Otherwise, scala/scala3#19702 will keep haunting us.
@lefou @lolgab can you take a look and check if I'm maybe missing some simple workaround?

@lihaoyi
Copy link
Member

lihaoyi commented Feb 16, 2024

I think the basic issue here is we allow import $ivy.blah, which desugars at runtime into import $ivy.$ and some other stuff. The "other stuff" is what's important, the import $ivy.$ is just a placeholder stub that does nothing

I think it should be possible to desugar import $ivy.blah into just whitespace . The import $ivy.$ (and similar) stubs don't actually do anything, so removing them should be ok

@Gedochao
Copy link
Contributor Author

Corresponding Ammonite issue:

@Gedochao
Copy link
Contributor Author

Hey, can someone help me with this?
Or should I extract 3.3.2 to a separate PR for now?
CI seems to be fine on 3.3.2, I guess, although I'd rather have it done in one go...
cc @lihaoyi @lefou

@lefou
Copy link
Member

lefou commented Mar 2, 2024

Yeah, I think splitting this up into a Scala 3.3.x update and an separate 3.4.x is the best way to make progress.

@lefou lefou changed the title Scala 3.3.2 & 3.4.0 Support for Scala 3.4.0 Mar 6, 2024
@Ichoran
Copy link
Contributor

Ichoran commented Jun 3, 2024

It's a little hard to tell what is still needed to get this working. All of my code depends on 3.4 features, and Jupyter needs Almond, and Almond depends on Ammonite, so...basically I can't use Jupyter notebooks until this gets fixed. I also depend on Java 21 (for Loom threads), and I notice that this PR stops testing even Java 17. Is this because the matrix is too big?

Anyway, I have time to help resolve the issue myself, but I can't tell right now what still needs doing. I can't see failing logs with the test, and I don't really want to dive in without knowing what I'm getting myself into.

@lihaoyi
Copy link
Member

lihaoyi commented Jun 4, 2024

It seems the latest scala 3 added bakc support for $. Maybe it means we dont need any code changes and can just skip the problematic 3.4.x versions?

@Ichoran
Copy link
Contributor

Ichoran commented Jun 4, 2024

That might be the thing to do for now, but we can't rely on anything with a trailing $ for the reason Nicolas mentions in the ad-hoc patch (which is supposed to be temporary): scala/scala3#19705

But I think you're right that it's in. 3.4.2, I think, so maybe the thing to do is just go with that and skip 3.4.0 and 3.4.1.

(But at least the $-alone needs to be fixed eventually. I don't see why $ivy and such shouldn't work if the compiler people decide to permit it.)

lihaoyi pushed a commit that referenced this pull request Jun 10, 2024
Added 3.4.2 to targets in a fairly straightforward, formulaic fashion.
3.4.0 and 3.4.1, for good reason, fail to parse `$` as a class file, but
a [temporary ad-hoc workaround was added in
3.4.2](scala/scala3#19705), so this needn't
block 3.4 support for now. Reliance on `object $` still needs to be
[fixed in
Ammonite](#1395).

Several internals changes required fixes also; I used the most obvious
solution (e.g. if Y extends X and Y is now private, use X instead), but
someone who knows more might want to read it over.

Tests pass on my machine for 3.3 and 3.4.2 (under Java 21).

If accepted, this patch would supersede
#1394
@lihaoyi lihaoyi closed this Jun 12, 2024
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

Successfully merging this pull request may close these issues.

4 participants