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

Specify impl for Show[Symbol] #4490

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

LeifW
Copy link

@LeifW LeifW commented Aug 2, 2023

On 2.12, Symbol.toString is e.g. 'foo. On 2.13, it's Symbol(foo).

I want to use Show to get away from the vagaries of relying on .toString - to have a reliable nailed-down String representation for types.

Want to use Show to build a pretty-printer, without having the output depend on which Scala version you're using.

I tried overriding the Show[Symbol] instance locally, but I just got an ambiguous implicit error. Don't know how to not import this instance, since it's on the companion object for Show.

On 2.12, `Symbol.toString` is e.g. `'foo`. On 2.13, it's `Symbol(foo)`.

I want to use `Show` to get away from the vagaries of relying on
`.toString` - to have a reliable nailed-down String representation for
types.

Want to use `Show` to build a pretty-printer, without having the output
depend on which Scala version you're using.

I tried overriding the `Show[Symbol]` instance locally, but I just got
an ambiguous implicit error. Don't know how to not import this instance,
since it's on the companion object for `Show`.
@@ -25,5 +25,5 @@ import cats.Show

trait SymbolInstances extends cats.kernel.instances.SymbolInstances {
implicit val catsStdShowForSymbol: Show[Symbol] =
Show.fromToString[Symbol]
Show.show("'" + _.name)
Copy link
Author

Choose a reason for hiding this comment

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

Or maybe this should just be .name - without the leading '? I think of Show as being something nice / sane for humans to read - we have .toString for diagnostic / debug output...

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this isn't safe since Symbol("foo bar") is allowed I think.

Instead I think we should check if the name is a valid Symbol literal (somehow, maybe regex?) and then we could use the '

Copy link
Author

@LeifW LeifW Aug 2, 2023

Choose a reason for hiding this comment

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

Not understanding what the issue is - what does being a valid 2.12 Symbol literal have to do with the Show representation? Is there a restriction that Show has to emit valid Scala source code? It already breaks that on the Show representation for String. 'foo bar is a valid string. That's the .toString of Symbol in Scala 2.12.
All this PR does is fix the Show implementation to the version already present on 2.12 - by inlining 2.12's definition of Symbol.toString: https://github.com/scala/scala/blob/2.12.x/src/library/scala/Symbol.scala#L30

Copy link
Contributor

Choose a reason for hiding this comment

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

My concern is that imagine you interpolate this into a string, it would be ambiguous even to a human where the symbol ends.

As such I think it is a bad default.

Show is a lawless typeclass anyway, so I think there is an excellent argument it shouldn't exist in cats but maybe alleycats.

I'd rather see a typeclass more about encoding and decoding into strings. Like a pretty printer + parser. Such a typeclass can be lawful.

Here it's all bikeshed it seems. So I totally understand if you prefer a different bikeshed color.

Copy link
Author

@LeifW LeifW Aug 3, 2023

Choose a reason for hiding this comment

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

This is literally already the current Show[Symbol] impl on 2.12. Having different goals for Show seems outside the scope of this PR. I don't care what it is, so long as it's consistent. Do you have any alternate suggestions? 2.13's Symbol(foo)? Or no decoration, just foo?
On Haskell there's perhaps a convention of read . show = id, but I don't see either of those typeclasses mention that explicitly, Read is bad anyways, and there's no Read in Cats. I can't see any "rules" for what the expected Show output is - I gather it's just intended to give a string intended for human readability. I gather the convention is to roughly follow existing .toString representations. I see Kittens adds labels to case class parameters - still looks almost like Scala source. Are you proposing a novel string representation for Symbols, or just that Show shouldn't exist here in the first place, or...?
There are much better solutions if you expect machine-parsible output - the current Show impls break any hope of that all over the place already (e.g. strings are not quoted).
Redesigning the entire Show typeclass / documentation with the expectation of machine parseability isn't something I'm interested in (especially not for this PR).
I'm not proposing a different bikeshed color, I just want it to be the same across Scala versions.
Your concern about a human "not being able to tell where the symbol ends" already exists in Show and .toString for Symbol on 2.12, as well as Show[Symbol] for all Scala versions, etc. See the Show output at the end of the box on https://github.com/typelevel/kittens#derive-show.
That Show output is not valid Scala source code, nor is it intended to be. It's a human-readable representation of the value it was called on.

Copy link
Author

@LeifW LeifW Jun 7, 2024

Choose a reason for hiding this comment

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

As far as the Show instance for Symbol, would you prefer:

  • 'foo bar (the current impl for 2.12)
  • Symbol(foo bar) (the current impl for 2.13)
  • conditional depending on if it has a space, so sometimes 'foo and sometimes Symbol(foo bar)?
  • something else?

The only thing his PR is trying to do is to make that impl consistent across Scala versions. I'm not interested in redesigning / removing the Show typeclass on this PR.

Copy link
Author

Choose a reason for hiding this comment

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

Related (there's no Read typeclass - Show is for human-readable output, not intended to be machine-parseable): #932 (comment)

@armanbilge
Copy link
Member

I tried overriding the Show[Symbol] instance locally, but I just got an ambiguous implicit error. Don't know how to not import this instance, since it's on the companion object for Show.

This should be solvable, can you create a small example that demonstrates the problem?

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.

3 participants