Skip to content

Warn if interpolator uses toString #20578

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

Merged
merged 8 commits into from
Apr 9, 2025
Merged

Conversation

som-snytt
Copy link
Contributor

Ports scala/scala#10776

"I always forget how to use Scala 3."

@som-snytt som-snytt marked this pull request as ready for review August 22, 2024 19:31
@SethTisue
Copy link
Member

The Scala 2 version of this will ship very soon (in 2.13.15).

@som-snytt som-snytt closed this Sep 15, 2024
@lenguyenthanh
Copy link

lenguyenthanh commented Oct 14, 2024

@som-snytt may I ask you why did you close this pr? I would love to use this flag 🙏

@som-snytt som-snytt reopened this Apr 4, 2025
@som-snytt som-snytt force-pushed the port/warn-toString branch from a2ebaa0 to 941615f Compare April 4, 2025 18:06
@Gedochao Gedochao requested a review from Linyxus April 7, 2025 06:27
@Gedochao Gedochao requested a review from KacperFKorban April 7, 2025 06:28
Copy link
Member

@KacperFKorban KacperFKorban 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 overall! I added some minor comments.

case _ => true

def lintToString(arg: Type): Unit =
if ctx.settings.Whas.toStringInterpolated && kind == StringXn && !(arg.widen =:= defn.StringType) && !arg.isPrimitiveValueType
then warningAt(CC)("interpolation uses toString")
Copy link
Member

Choose a reason for hiding this comment

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

This would probably be better, when done as a separate PR, but it might be useful to add message ids for all the error/warning messages in this file.

@som-snytt som-snytt force-pushed the port/warn-toString branch from 941615f to 6fc84a6 Compare April 7, 2025 17:47
@som-snytt
Copy link
Contributor Author

som-snytt commented Apr 7, 2025

I took the suggestions, and added an error ID and kind for the messages, which look better that way. (Such a small change to make such a big difference.) (Probably f"" isn't used all that much for reasons of efficiency, so maybe the difference is not great in the field.)

There are too many specific messages for more granularity.

Also minor refactoring or formatting for readability. Some of the long one-liners look like it's trying to save paper for a print-out.

@som-snytt som-snytt force-pushed the port/warn-toString branch 2 times, most recently from 295bcc9 to d96843b Compare April 7, 2025 22:18
@som-snytt som-snytt force-pushed the port/warn-toString branch from d96843b to ba4204e Compare April 8, 2025 13:31
@som-snytt som-snytt merged commit 7976598 into scala:main Apr 9, 2025
29 checks passed
@som-snytt som-snytt deleted the port/warn-toString branch April 9, 2025 00:19
@WojciechMazur WojciechMazur added this to the 3.7.1 milestone May 1, 2025
@WojciechMazur WojciechMazur added the release-notes Should be mentioned in the release notes label May 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes Should be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants