Rename Expr.valueOr{Error => Throw}

[japgolly]

Tiny change to improve clarity a bit. Error is a noun and it's a legitimate interpretation to think "oh, maybe this can return an error". With Throw it's clear and unambiguous that an exception will be thrown.

[nicolasstucki]

To be consistent with the error reporting API it should be valueOrThrowError. This distinction is important because this kind of exception is handled in a special case inline error reporting. Another exception would be reported with its stack trace.

[japgolly]

@nicolasstucki Sure! So is changing everything to xxxOrThrowError enough? Or is there something else I need to do to address that special case you were talking about?

[nicolasstucki]

If 3.0.0-RC2 gets bumped to 3.0.0 then we should have >3.0.0. It would probably be 3.1.0 as we this is a binary incompatible change.

[japgolly]

Maybe we keep this PR open until we're 100% sure about the plans for RC3? Then I can update this to either 3.0.0-RC2 or 3.0.0

[japgolly]

Actually, I just set this to 3.0.0 so that it's ready to go if it's correct. (Other PR feedback addressed.)

[abgruszecki]

How about valueOrAbort? It seems to communicate the intent to abort better and is shorter to type. valueOrPanic works as well, if we want to borrow Rust terminology.

[nicolasstucki]

I do like Abort, but then we should also change the methods in report.

[japgolly]

I think abort is less clear because "abort" just means halt/cancel a process. It doesn't indicate how. For example, someone might think it will abort by returning null. What I like about throw is that it unambiguously aligns with the JVM terminology for raising an exception.

[nicolasstucki]

To be consistent we still need to have a name that coincides with error reporting. Throwing is only half of what the users need to know about this method. I believe the best option we have to keep everything consistent and clear is to rename as follows

• valueOrError -> valueOrErrorAndThrow
• report.{throwError -> errorAndThrow}

The advantages of this naming are

• Consistent name suffixes xxxErrorAndThrow
• Both the error reporting and the throwing are explicitly stated
• report.e will show autocompletion for all variants of error and errorAndThrow
• The longer name of valueOrErrorAndThrow will help users to use value instead and handle the error with nicer domain-specific error messages.

--  def valueOrError(using FromExpr[T]): T =
++  def valueOrErrorAndThrow(using FromExpr[T]): T =
      val fromExpr = summon[FromExpr[T]]
      def reportError =
        val msg = s"Expected a known value. \n\nThe value of: ${self.show}\ncould not be extracted using $fromExpr"
--      reflect.report.throwError(msg, self)
++      reflect.report.errorAndThrow(msg, self)
      given Quotes = Quotes.this
      fromExpr.unapply(self).getOrElse(reportError)

This is a change that should be backported to the release-3.0.0 branch after it is merged.

[nicolasstucki]

Suggested change

	def valueOrError(using e: FromExpr[T]): T =
	def valueOrError(using FromExpr[T]): T =

[abgruszecki]

I think abort is less clear because "abort" just means halt/cancel a process. It doesn't indicate how. For example, someone might think it will abort by returning null. What I like about throw is that it unambiguously aligns with the JVM terminology for raising an exception.

No reasonable person will think that a computation can abort by returning null. Like you say, methods are read like verbs, in context. When I call a method named valueOrAbort, I expect that method to either give me a value, or abort my computation. If the meaning of "abort" isn't clear to me, I can always peek at the docstring of the method and find out.

Likewise, valueOrErrorAndThrow is just a step down from valueOrError. The name is just confusing - what will this method do, simultaneously return me an error value and throw? I don't really want to say this, but this reminds me of 'AbstractCloneablePOJOBeanFactory`-style naming conventions.

The reason why I'm saying "use a different verb than throw" is that within a single API, one is free to use some bespoke terminology. If within that API, "aborting" always means "throw an exception that is intended to be caught by whatever is running quotes", then we get a method that has a succinct name, tells familiar users everything they know, and makes unfamiliar users look at the method docstring and learn something about the API.

[nicolasstucki]

This naming change should not be rushed. We can do it after 3.0.

[japgolly]

The reason why I'm saying "use a different verb than throw" is that within a single API, one is free to use some bespoke terminology. If within that API, "aborting" always means "throw an exception that is intended to be caught by whatever is running quotes", then we get a method that has a succinct name, tells familiar users everything they know, and makes unfamiliar users look at the method docstring and learn something about the API.

@abgruszecki That logic doesn't make sense to me at all. "Throw" is clear because it will throw an exception. There's no other interpretation of that. These ideas that using ambiguous terminology is good because it will encourage people to read the docs is unrealistic and exactly the opposite of what I believe the goal should be. Why not just choose the 100% clear verb?

[abgruszecki]

@japgolly let me quote myself:

within that API, "aborting" always means "throw an exception that is intended to be caught by whatever is running quotes"

With Expr#valueOrX, we have an optional value that genuinely might not be there, and we really want to just abort current inlining if it isn't. If we name this method valueOrAbort, then we can use the OrThrow suffix for methods that, say, throw an exception that is intended to be caught by the user, not by the platform.

Also, I'd expect that the vast majority of Scala users can jump to a method's definition with a single click.

[japgolly]

@abgruszecki Ah ok I see what you're saying. So if it's abort the compiler handles the abortion; if it's throw the user should catch the exception. I'm glad I now understand your argument but I'm not sure I agree with you because the compiler should be catching and handling every exception thrown in quoting API. I don't think there would ever be a case where the user has to catch an exception because the compiler wont (and will crash if uncaught).

Also, I'd expect that the vast majority of Scala users can jump to a method's definition with a single click.

Woah noooo no no no no I'd put that in the false assumptions bucket if I were you. Metals and to a lesser degree Intellij, very regularly fail to jump to the source of stuff on the classpath for me. Sometimes I waste 20 minutes trying to re-import and restart everything, and clear out what cache directories I can find. I'm a regular counter-example to assuming this works for all people all the time, and I can't imagine I'm the one person in the world who experiences these problems. I've been writing Scala for around 10 years and it's consistently been my experience that inline documentation is completely broken and unfortunately, ultimately useless. What's really great is that Metals keeps getting better and better, and ScalaDoc looks to be getting some proper love for Scala 3, so I'm optimistic about the future! Sadly we're not there yet though, which is why I want to add some caution to your assumption.

[nicolasstucki]

Also, I'd expect that the vast majority of Scala users can jump to a method's definition with a single click.

Woah noooo no no no no I'd put that in the false assumptions bucket if I were you. Metals and to a lesser degree Intellij, very regularly fail to jump to the source of stuff on the classpath for me. Sometimes I waste 20 minutes trying to re-import and restart everything, ...

The alternative is to take 30 seconds out of the IDE and just look at the docs online. This is a method of the standard library and it does not depend on the project build anyway.

[nicolasstucki]

We are probably going with abort to avoid concept overloading. We will use #12056 as it also includes some other improvements.