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.

Sign up for GitHub

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

Jump to bottom

Concatenate strings if not formatting [ci: last-only] #10364

Merged
merged 9 commits into from Jul 10, 2023
Merged

Concatenate strings if not formatting [ci: last-only] #10364

merged 9 commits into from Jul 10, 2023

Conversation

som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Apr 4, 2023 •
edited

@scala-jenkins scala-jenkins added this to the 2.13.12 milestone Apr 4, 2023
@som-snytt som-snytt force-pushed the issue/9784-f-to-s branch 2 times, most recently from 59f62b5 to f88e10d Compare April 5, 2023 05:56
@som-snytt
Copy link
Contributor Author

The current caret position is unusual.

test/files/neg/t8650.scala:12: error: annotation argument needs to be a constant; found: "".+("hello").+(" world")
  @deprecated(s"$greeting world", since=s"$lib $version")
                         ^
test/files/neg/t8650.scala:12: error: annotation argument needs to be a constant; found: "".+("MyLib").+(" ").+("17")
  @deprecated(s"$greeting world", since=s"$lib $version")
                                                ^

I saw that the ancient PR requested constfold directly.

@sjrd sjrd dismissed their stale review April 5, 2023 14:52

Portability concerns addressed.

@som-snytt som-snytt changed the title Concatenate if not formatting Concatenate strings if not formatting Apr 5, 2023
@som-snytt som-snytt mentioned this pull request Apr 5, 2023
Closed
@som-snytt
Copy link
Contributor Author

Random presentation/doc failure?

warning: 1 deprecation (since 2.13.0); re-run with -deprecation for details
reload: Base.scala, Class.scala, Derived.scala
Unexpected foo method comment:None

@som-snytt som-snytt marked this pull request as ready for review April 11, 2023 02:13
@som-snytt
Copy link
Contributor Author

Just noticed https://prog21.dadgum.com/76.html "Stop the Vertical Tab Madness" via "things rust shipped without" https://graydon2.dreamwidth.org/218040.html "vertical tab escapes along with the escapes for bell and form-feed".

"\n" almost always means s"$lineSeparator", so it would be nice if that were not a wart.

lrytz
lrytz reviewed Jul 7, 2023
View reviewed changes
Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

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

Looks good to me otherwise.

src/compiler/scala/tools/reflect/FormatInterpolator.scala
@@ -94,6 +98,7 @@ abstract class FormatInterpolator {
else all.head + all.tail.map { case req(what) => what case _ => "?" }.mkString(", ", ", ", "")
}
c.error(args(argi).pos, msg)
reported = true
Copy link
Member

Choose a reason for hiding this comment

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

Could reported be a local variable next to formatting? It's not obvious when instances of FormatInterpolator are created.

There are also some invocations of c.error in escapeHatch - does that matter?

Copy link
Contributor Author

@som-snytt som-snytt Jul 7, 2023 •
edited

Choose a reason for hiding this comment

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

The cached FastTrack mechanism results in a ctx => (new { val c = ctx } with FastStringInterpolator).interpolateF, so a new interpolator per invocation. Not obvious.

reported is set also by Conversion#errorAt, so that is why it's not local. The flag is needed so that it doesn't attempt a bad compiletime format, so the bad escapes in escapeHatch should matter. I'll make a test.

Edit: there is already a test; took me a while to realize that format() doesn't complain about bad escapes in a format string, Scala does.

@som-snytt som-snytt changed the title Concatenate strings if not formatting Concatenate strings if not formatting [ci: last-only] Jul 7, 2023
@som-snytt
Copy link
Contributor Author

Rebased, no squash, added a small test.

lrytz
lrytz approved these changes Jul 10, 2023
View reviewed changes
Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

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

👍 thank you!

@lrytz lrytz merged commit f2bdad0 into scala:2.13.x Jul 10, 2023
3 checks passed
@som-snytt som-snytt deleted the issue/9784-f-to-s branch July 10, 2023 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lrytz lrytz lrytz approved these changes

@sjrd sjrd sjrd left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.13.12
4 participants
@som-snytt @lrytz @sjrd @scala-jenkins