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

Nowarn fix for Whitespace macro #310

Merged
merged 17 commits into from
May 19, 2024
Merged

Nowarn fix for Whitespace macro #310

merged 17 commits into from
May 19, 2024

Conversation

He-Pin
Copy link
Contributor

@He-Pin He-Pin commented May 18, 2024

Motivation:
refs: #285
with reading: https://www.scala-lang.org/2021/01/12/configuring-and-suppressing-warnings.html

Modification:

  1. I updated some depenecies,both scala and etc
  2. because of url pattern need update mill#3167, I have to comments out the native modules locally
  3. add -Xlint:unused to reproduce the problem
  4. updated some tests when 3 is enabled.
  5. remove 2.11 related code
  6. as Scala 2.12.x now supports the nowarn , so I changed the code to use the @nowarn annotation directly.
  7. the root cause is the annotation in WhiteSpace macro

Result:
I think the issue is fixed now, and as I'm using Fastparse within a Java project, so I was not knowing this issue.
All tests passed locally.

@He-Pin He-Pin changed the title Nowarn fix Nowarn fix for Whitespace macro May 18, 2024
@He-Pin He-Pin force-pushed the nowarn branch 4 times, most recently from 168fb1e to 5d53ff6 Compare May 18, 2024 17:28
@He-Pin
Copy link
Contributor Author

He-Pin commented May 18, 2024

@fanf Would you like to give this a try? I mean locally.

@lihaoyi
Copy link
Member

lihaoyi commented May 18, 2024

The PR looks good to me, once you get CI green I can merge it and close out the bounty

@He-Pin
Copy link
Contributor Author

He-Pin commented May 19, 2024

@lihaoyi Would you like give it another round of run, thanks.

@He-Pin He-Pin force-pushed the nowarn branch 2 times, most recently from 745f2f6 to e271ffe Compare May 19, 2024 01:01
@He-Pin
Copy link
Contributor Author

He-Pin commented May 19, 2024

@lihaoyi The CI is happy now:)

Note: in Scala 2.12, I added two mina filters where the nowarn is the scala nowarn now.

@lihaoyi
Copy link
Member

lihaoyi commented May 19, 2024

@He-Pin can we leave the original nowarn in place but deprecated? That way we can avoid adding the mima filters

@He-Pin
Copy link
Contributor Author

He-Pin commented May 19, 2024

@lihaoyi OK, I think that would be better.

@He-Pin
Copy link
Contributor Author

He-Pin commented May 19, 2024

@lihaoyi I have updated and removed the added mima.

@@ -0,0 +1,61 @@
package scalaparse
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@He-Pin is this change intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because the api in nsc is different in Scala 2.12 and newer version of scala 2.13.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I have to separate this test for Scala 2.12 and Scala 2.13, maybe I can split it into a dedicated PR. but as the commits is one by one, maybe that's fine too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

related link: scala/scala#10406


override def incompleteInputError(msg: String, actions: List[CodeAction]): Unit = {
fail = true
super.incompleteInputError(msg, actions)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the new api has a actions


override def syntaxError(offset: global.syntaxAnalyzer.Offset, msg: String, actions: List[CodeAction]): Unit = {
fail = true
super.syntaxError(offset, msg, actions)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so does syntaxError method. otherwise, it will report override nothing error.

@lihaoyi
Copy link
Member

lihaoyi commented May 19, 2024

@He-Pin not sure if I'm missing something, but where is the new version of ScalacParser used in the PR?

@He-Pin
Copy link
Contributor Author

He-Pin commented May 19, 2024

@lihaoyi Because I updated the Scala 2.13.x to 2.13.14 in this PR, and with scala/scala#10406 , the additional actions: List[CodeAction] parameter is added, so when do cross compiling with Scala 2.12 and Scala 2.13, it will raise an error override nothing without this change in the scalaparse tests.

val scalaJS1 = "1.12.0"
val scalaNative04 = "0.5.0"
val scala3 = "3.3.3"
val scala213 = "2.13.14"
Copy link
Contributor Author

@He-Pin He-Pin May 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's now 2.13.14, and the nsc is updated scala/scala#10406

@lihaoyi cc

@lihaoyi
Copy link
Member

lihaoyi commented May 19, 2024

@He-Pin got it, looks good! Can send me details at [email protected] and I'll close out the bounty

@lihaoyi lihaoyi merged commit 7f6dddc into com-lihaoyi:master May 19, 2024
5 checks passed
@He-Pin He-Pin deleted the nowarn branch May 19, 2024 06:45
@He-Pin
Copy link
Contributor Author

He-Pin commented May 19, 2024

thanks, I just implement a jsonpath (https://www.rfc-editor.org/rfc/rfc9535#name-collected-abnf-grammars) with Fastparse at work, it works great.

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.

2 participants