Skip to content

Enhance constant-folding during inlining (second try)#25731

Merged
mbovel merged 1 commit into
scala:mainfrom
dotty-staging:mb/inline-val-constant
Jul 13, 2026
Merged

Enhance constant-folding during inlining (second try)#25731
mbovel merged 1 commit into
scala:mainfrom
dotty-staging:mb/inline-val-constant

Conversation

@mbovel

@mbovel mbovel commented Apr 8, 2026

Copy link
Copy Markdown
Member

#24431 was reverted in #25703. This PR restores #24431.

Fixes #24420 and fixes #24412.

Comment thread compiler/src/dotty/tools/dotc/inlines/Inliner.scala
@mbovel

This comment was marked as outdated.

@mbovel

This comment was marked as outdated.

mbovel added a commit to mbovel/oolong that referenced this pull request Apr 8, 2026
When the compiler constant-folds `Pattern.CASE_INSENSITIVE | Pattern.COMMENTS`
into a single IntConstant (e.g. 6), parseFlags failed to match. This adds a
case to decompose the constant back into individual flag bits.

See scala/scala3#25731

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@sjrd

sjrd commented Apr 8, 2026

Copy link
Copy Markdown
Member

An ascription that doesn't pass that condition is an invalid ascription. How does it even get there?

@mbovel

mbovel commented Apr 8, 2026

Copy link
Copy Markdown
Member Author

An ascription that doesn't pass that condition is an invalid ascription. How does it even get there?

It turns out you can generate malformed trees with macros. See #25690 for more 😅

@sjrd

sjrd commented Apr 8, 2026

Copy link
Copy Markdown
Member

Right. Then the macro needs to be fixed.

@mbovel

mbovel commented Apr 8, 2026

Copy link
Copy Markdown
Member Author

Right. Then the macro needs to be fixed.

Sure, that's why I opened the PR on ZIO to enable -Xcheck-macros and hopefully find the problem. But for now that just triggers another compiler crash.

In the meantime, and in case other codebases rely on broken macros, don't you think that defensively checking the assertion is valid would make sense?

@mbovel

mbovel commented Apr 8, 2026

Copy link
Copy Markdown
Member Author

See also the internal Slack discussion about -Xcheck-macros: https://lampepfl.slack.com/archives/C06FYNL1WDT/p1775647016206789.

@sjrd

sjrd commented Apr 8, 2026

Copy link
Copy Markdown
Member

don't you think that defensively checking the assertion is valid would make sense?

No, it really doesn't make sense. You can't pollute every other place of the compiler with dealing with bad stuff that macros generate. It's not sustainable. In this case it might run on the JVM because it inserts casts to heal things, but not on JS, whose backend is stricter.

At most, you could have a postprocessing after a macro that turns bad ascriptions into casts. And emits a warning in the process. At least the blast radius of that hack would be limited to macros.

@mbovel

mbovel commented Apr 8, 2026

Copy link
Copy Markdown
Member Author

I filled zio/zio#10721, let's see if I can get some help fixing this in ZIO directly.

PR also opened: zio/zio#10725.

danslapman pushed a commit to mbovel/oolong that referenced this pull request Apr 9, 2026
When the compiler constant-folds `Pattern.CASE_INSENSITIVE | Pattern.COMMENTS`
into a single IntConstant (e.g. 6), parseFlags failed to match. This adds a
case to decompose the constant back into individual flag bits.

See scala/scala3#25731

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
danslapman pushed a commit to mbovel/oolong that referenced this pull request Apr 9, 2026
When the compiler constant-folds `Pattern.CASE_INSENSITIVE | Pattern.COMMENTS`
into a single IntConstant (e.g. 6), parseFlags failed to match. This adds a
case to decompose the constant back into individual flag bits.

See scala/scala3#25731

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
danslapman pushed a commit to mbovel/oolong that referenced this pull request Apr 9, 2026
When the compiler constant-folds `Pattern.CASE_INSENSITIVE | Pattern.COMMENTS`
into a single IntConstant (e.g. 6), parseFlags failed to match. This adds a
case to decompose the constant back into individual flag bits.

See scala/scala3#25731

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
danslapman pushed a commit to leviysoft/oolong that referenced this pull request Apr 9, 2026
When the compiler constant-folds `Pattern.CASE_INSENSITIVE | Pattern.COMMENTS`
into a single IntConstant (e.g. 6), parseFlags failed to match. This adds a
case to decompose the constant back into individual flag bits.

See scala/scala3#25731

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@mbovel
mbovel force-pushed the mb/inline-val-constant branch from 81b7a7e to 05083ab Compare April 20, 2026 20:17
@mbovel

mbovel commented Apr 20, 2026

Copy link
Copy Markdown
Member Author

@WojciechMazur, I made zio/zio#10725 on ZIO to avoid #25690 and #25692, and leviysoft/oolong#61 on Oolong to avoid #25697, so I hope the open community build can now pass on this change. Could you please update ZIO and Oolong in the open community build, and then run it on this PR?

@WojciechMazur

WojciechMazur commented Apr 21, 2026

Copy link
Copy Markdown
Contributor

Could you please update ZIO and Oolong in the open community build, and then run it on this PR?

Sure, just be aware I can force these 2 to be used from main branch instead of latest release, but when it comes to zio the problem was in majority present for the users of their macros - I cannot safely force zio dependency upgrade for these, as it might lead to other, unrelated compilation errors. Effectively their builds might be broken until they upgrade to latest version of ZIO

@mbovel

mbovel commented Apr 21, 2026

Copy link
Copy Markdown
Member Author

Could you please update ZIO and Oolong in the open community build, and then run it on this PR?

Sure, just be aware I can force these 2 to be used from main branch instead of latest release, but when it comes to zio the problem was in majority present for the users of their macros - I cannot safely force zio dependency upgrade for these, as it might lead to other, unrelated compilation errors. Effectively their builds might be broken until they upgrade to latest version of ZIO

Ah, I see, thanks for the explanation. So I guess I need to wait until both ZIO and Oolong release new versions.

@mbovel

mbovel commented May 6, 2026

Copy link
Copy Markdown
Member Author

@WojciechMazur ZIO just released 2.1.26 with my fix 🥳
And for Oolong, my fix is included in v0.5.1.

Could we try running the community build on this PR, with these updated versions?

@mbovel
mbovel force-pushed the mb/inline-val-constant branch from 05083ab to bbd3d41 Compare May 10, 2026 16:08
@mbovel

mbovel commented May 18, 2026

Copy link
Copy Markdown
Member Author

If I understand correctly, the open community build actually uses the latest releases by default.

So it might pass now, modulo projects that might use older versions of ZIO, as Wojciech mentioned above.

Solal launched a community build run for me:

Wait and see.

@mbovel

mbovel commented May 19, 2026

Copy link
Copy Markdown
Member Author

Results: https://scala3.westeurope.cloudapp.azure.com/dashboard/compare?baseScalaVersion=3.9.0-RC1-bin-20260508-eb2e041-NIGHTLY&targetScalaVersion=3.9.0-RC1-bin-20260510-bbd3d41.

It shows 88 new failing projects, but it seems a lot of them are not related to this PR 🤔

@mbovel

mbovel commented May 19, 2026

Copy link
Copy Markdown
Member Author

@WojciechMazur: the community build took oolong 0.5.0, while the latest version on Scaladex and Maven central is 0.5.2. Do you know why? That's actually the only new failure caused by a compilation error.

The other 87 new failures are due to tests failing. It's still not clear to me how these are related to this PR.

@WojciechMazur

Copy link
Copy Markdown
Contributor

the community build took oolong 0.5.0, while the latest version on Scaladex and Maven central is 0.5.2. Do you know why?

It caused by problems with our Scaladex scrapping, resulting in project configs not being updated. I'm fixing it right now

@mbovel
mbovel force-pushed the mb/inline-val-constant branch from bbd3d41 to 22f1319 Compare May 28, 2026 16:26
@mbovel

mbovel commented Jun 1, 2026

Copy link
Copy Markdown
Member Author

@WojciechMazur, ping me when I can try to run the open community build again 🙂

@WojciechMazur
WojciechMazur force-pushed the mb/inline-val-constant branch from 22f1319 to 387126e Compare July 7, 2026 11:03
@WojciechMazur

WojciechMazur commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

There are conflicts in the presentation compiler build - once it's fixed we can shcedule a new OpenCB
I've rebased the branch previously so we can have an up to date results and compare against latest nightlies reliably

@mbovel
mbovel force-pushed the mb/inline-val-constant branch from 387126e to 66263c2 Compare July 8, 2026 12:26
@WojciechMazur

WojciechMazur commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

OpenCB results against 08.07.2026 nightly (last one) https://scala3.westeurope.cloudapp.azure.com/dashboard/compare?baseScalaVersion=3.10.0-RC1-bin-20260707-a4dab1a-NIGHTLY&targetScalaVersion=3.10.0-RC1-bin-20260708-66263c2&reason=Compilation
Seems to be no new regressions, but be aware that revently there was a regression introduced on main that caused ~80 failures and was not fixed yet, so there might be a false-negatives in this comparsion

@mbovel

mbovel commented Jul 8, 2026

Copy link
Copy Markdown
Member Author

Thanks a lot!
Do you think we should wait for the regression on main to be fixed then, or merge this PR already?

@mbovel

mbovel commented Jul 9, 2026

Copy link
Copy Markdown
Member Author

I see no regression in pekko or zio-dependent libraries, so I think we can try to merge and see if we get any regressions in the next nightlies.

@mbovel
mbovel requested a review from SolalPirelli July 9, 2026 19:09
@SolalPirelli

Copy link
Copy Markdown
Contributor

It looks okay to me but I'm not the right person for inlining stuff, maybe @jchyb ?

@SolalPirelli
SolalPirelli removed their request for review July 10, 2026 09:35
@SolalPirelli SolalPirelli assigned mbovel and unassigned WojciechMazur Jul 10, 2026
@mbovel

mbovel commented Jul 10, 2026

Copy link
Copy Markdown
Member Author

It looks okay to me but I'm not the right person for inlining stuff, maybe @jchyb ?

Sure, but you already approved that exact changes set in #24431, that's why I asked you 😄

@SolalPirelli

Copy link
Copy Markdown
Contributor

Ah I forgot this PR had history... okay given the previous discussion I guess people are okay with it.

@SolalPirelli
SolalPirelli force-pushed the mb/inline-val-constant branch from 66263c2 to d3632c4 Compare July 10, 2026 09:41
@SolalPirelli

Copy link
Copy Markdown
Contributor

I'm rebasing because main is already broken and we shouldn't break it further so let's wait til that's fixed to merge this.

@SolalPirelli SolalPirelli added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Jul 10, 2026
@SolalPirelli SolalPirelli added this to the 3.9.0 milestone Jul 10, 2026
@Gedochao
Gedochao force-pushed the mb/inline-val-constant branch from d3632c4 to 064c25e Compare July 13, 2026 10:17
@Gedochao

Copy link
Copy Markdown
Contributor

(rebased)

@mbovel
mbovel merged commit 8433159 into scala:main Jul 13, 2026
49 checks passed
@mbovel
mbovel deleted the mb/inline-val-constant branch July 13, 2026 11:30
@WojciechMazur WojciechMazur added the release-notes Should be mentioned in the release notes label Jul 19, 2026
@WojciechMazur

Copy link
Copy Markdown
Contributor

@mbovel Regression reproducer for kitlangton/neotype, do you think it must be fixed in the library or can be resolved in compiler?

// macro.scala

import scala.quoted.*

abstract class Newtype[A]:
  opaque type Type = A

  inline def apply(inline input: A): Type =
    ${ Newtype.applyImpl[A, Type]('input) }

object Newtype:
  def applyImpl[A: Type, T: Type](input: Expr[A])(using Quotes): Expr[T] =
    // Same cast neotype uses after (optional) compile-time validation.
    input.asExprOf[T]
//> using file macro.scala

type NonEmptyString = NonEmptyString.Type
object NonEmptyString extends Newtype[String]

type FullName = FullName.Type
object FullName extends Newtype[NonEmptyString]

@main def Test(): Unit =
  val fullName: FullName = FullName(NonEmptyString("Bob Smith"))
  println(fullName)

yields

Compiling project (Scala 3.10.0-RC1-bin-20260718-b17ee7a-NIGHTLY, JVM (25))
[error] ./test.scala:19:28
[error] Macro expansion has type String, which does not conform to the expected type NonEmptyString
[error]   val fullName: FullName = FullName(NonEmptyString("Bob Smith"))
[error]                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Error compiling project (Scala 3.10.0-RC1-bin-20260718-b17ee7a-NIGHTLY, JVM (25))

Potential fix:
replace input.asExprOf[T] with

object Newtype:
  def applyImpl[A: Type, T: Type](input: Expr[A])(using Quotes): Expr[T] =
    import quotes.reflect.*
    Typed(input.asTerm, TypeTree.of[T]).asExprOf[T]

however it feels like that can be something compiler can do for us instead. There was no issues under -Xcheck-macros before

@mbovel

mbovel commented Jul 19, 2026

Copy link
Copy Markdown
Member Author

@WojciechMazur the fix is the same as in https://github.com/scala/scala3/pull/25731/changes#diff-1afc6b9cbf71fc548e1526eea8be0e3420423fd7da2485caebd5473f055ac125; use an asInstanceOf in the generated code, not at the meta level:

// macro.scala

import scala.quoted.*

abstract class Newtype[A]:
  opaque type Type = A

  inline def apply(inline input: A): Type =
    ${ Newtype.applyImpl[A, Type]('input) }

object Newtype:
  def applyImpl[A: Type, T: Type](input: Expr[A])(using Quotes): Expr[T] =
    // Same cast neotype uses after (optional) compile-time validation.
    '{$input.asInstanceOf[T]}

There is a crucial phase distinction here:

  • Performing an unsafe cast in the generated code is fine.
  • Performing an unsafe (and illegal) cast at the meta level, on quotes, must be a hard error. Otherwise, we would authorize an ill-typed tree in the compiler.

Previously, -Xcheck-macros did not report an error because this check was missing. However, allowing such casts resulted in compiler crashes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. release-notes Should be mentioned in the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

inline val not inlined inline boolean value is not simplified in some cases

5 participants