Skip to content

Fix Scala 3 Macros #26

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

Closed
wants to merge 1 commit into from
Closed

Conversation

Flowdalic
Copy link
Contributor

@Flowdalic Flowdalic commented Sep 13, 2023

The charSequenceExprToStringExpr() method caused (some?) strings to be evaluated to

'{ ... }

It turns out, the whole function is not necessary: every invocation of ${charSequenceExprToStringExpr(message)} can be replaced with ${message.toString}, which evaluates to the desired String (e.g., "{} failed because of {} (baz={})").

Furthermore, the return type of deconstructInterpolatedMessage() can be changed from Expr[String] to Expr[CharSequence]. This allows deconstructInterpolatedMessage() to drop its usage of charSequenceExprToStringExpr().

@pjfanning
Copy link
Contributor

I have no status in the Apache Log4j project so I can't say if anyone from that team will merge this. It's over 3 years since the last release and in a recent email thread, I suggested my preference is to abandon this project.

https://lists.apache.org/thread/wtjmrq94ql2vkovq16tpolcfnzd45qbp

The change looks potentially useful but it would be better if there was a unit test added that demos that this works. It might be better to wait to see if any Apache Log4j team member is willing to merge this and do releases before spending time improving this PR. If you do want to proceed, I would suggest a test that does something like this (in pseudocode):

var logCount: Int = 0

def incrementAndGetLogCount(): Int = {
  logCount += 1
  logCount
}

// in a test
logger.debug(s"logCount = ${incrementAndGetLogCount()}")
assert that logCount equals 0

The charSequenceExprToStringExpr() caused (some?) strings to be
evaluated to

'{ ... }

It turns out, the whole function is not necessary: every invocation
of ${charSequenceExprToStringExpr(message)} can be replaced with
${message.toString}, which evaluates to the desired String (e.g., "{}
failed because of {} (baz={})").

Furthermore, the return type of deconstructInterpolatedMessage() can
be changed from Expr[String] to Expr[CharSequence]. This allows
deconstructInterpolatedMessage() to drop its usage of
charSequenceExprToStringExpr().
vy added a commit that referenced this pull request Sep 29, 2023
@vy vy added this to the 13.0.0 milestone Sep 29, 2023
@vy vy self-assigned this Sep 29, 2023
@vy vy added the bug label Sep 29, 2023
@vy
Copy link
Member

vy commented Sep 29, 2023

@Flowdalic, thanks so much for the fix and apologies for the late response. I have incorporated your fix in 66ac4a7. I would really appreciate it if you can double check it. For the record, the project went through a major facelift operation. A sanity check from an experienced Scala developer will be great.

@vy
Copy link
Member

vy commented Sep 29, 2023

@pjfanning, I understand the reaction and apologies for the wait. I am trying to overhaul the project. I will really appreciate it if you can submit the test you shared in a separate PR.

@vy vy closed this Sep 29, 2023
@pjfanning
Copy link
Contributor

pjfanning commented Sep 29, 2023

@pjfanning, I understand the reaction and apologies for the wait. I am trying to overhaul the project. I will really appreciate it if you can submit the test you shared in a separate PR.

Could you provide build instructions? I tried maven build with Java 11 and fails saying I need Java 17. Tried Java 17 and it fails saying I need Java 8. Tried Java 8 and fails saying I need Java 17.

@pjfanning
Copy link
Contributor

I got the maven build to work using Java 17 and setting a toolchains.xml that had a link to a Java 8 install.

@vy
Copy link
Member

vy commented Sep 29, 2023

@pjfanning See the build instructions in src/site/index.adoc. (Sorry, the website is not up-to-date yet. I will do a release in a day or two.)

@pjfanning pjfanning mentioned this pull request Sep 29, 2023
Flowdalic added a commit to Flowdalic/logging-log4j-scala that referenced this pull request Nov 4, 2023
This adds a few simple and short tests for the Scala 3
macros. Especially for the central deconstructMessageFormat() macro,
which was previously broken and fixed with 66ac4a7 ("Fixed Scala
3 macros (apache#26)").
Flowdalic added a commit to Flowdalic/logging-log4j-scala that referenced this pull request Nov 6, 2023
This adds a few simple and short tests for the Scala 3
macros. Especially for the central deconstructMessageFormat() macro,
which was previously broken and fixed with 66ac4a7 ("Fixed Scala
3 macros (apache#26)").
Flowdalic added a commit to Flowdalic/logging-log4j-scala that referenced this pull request Nov 6, 2023
This adds a few simple and short tests for the Scala 3
macros. Especially for the central deconstructMessageFormat() macro,
which was previously broken and fixed with 66ac4a7 ("Fixed Scala
3 macros (apache#26)").

Signed-off-by: Florian Schmaus <flo@geekplace.eu>
Flowdalic added a commit to Flowdalic/logging-log4j-scala that referenced this pull request Nov 6, 2023
This adds a few simple and short tests for the Scala 3
macros. Especially for the central deconstructMessageFormat() macro,
which was previously broken and fixed with 66ac4a7 ("Fixed Scala
3 macros (apache#26)").

Signed-off-by: Florian Schmaus <flo@geekplace.eu>
vy pushed a commit that referenced this pull request Nov 6, 2023
This adds a few simple and short tests for the Scala 3 macros.
Especially for the central `deconstructMessageFormat()` macro,
which was previously broken and fixed in #26.

Signed-off-by: Florian Schmaus <flo@geekplace.eu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants