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

in Scaladoc, be clearer that asInstanceOf is unsafe (and platform-dependent) #10696

Merged
merged 2 commits into from Apr 9, 2024

Conversation

robstoll
Copy link
Contributor

@robstoll robstoll commented Feb 21, 2024

@scala-jenkins scala-jenkins added this to the 2.13.14 milestone Feb 21, 2024
@robstoll robstoll force-pushed the patch-3 branch 2 times, most recently from 46f084e to c226e09 Compare February 21, 2024 22:39
@SethTisue SethTisue added the documentation No code change. Only documentation label Feb 21, 2024
src/library-aux/scala/Any.scala Outdated Show resolved Hide resolved
@SethTisue
Copy link
Member

It's good to add these cautions, but perhaps it would also be good to give some indication of what the method is for.

@robstoll
Copy link
Contributor Author

robstoll commented Feb 22, 2024

@SethTisue I actually woke up this morning and thought the same 😄 without an explanation for what it is intended for, it makes it look like the method shouldn't be there in the first place.

I'll rework the doc, thanks for all input

@som-snytt
Copy link
Contributor

Abandon all hope, ye who enter here. is the traditional messaging.

@Sporarum
Copy link

I think the comment is a bit overly wordy
Notably the example of what happens on JDK8 is in my opinion superfluous
(I don't think we should sacrifice clarity for brevity, but in this case making it smaller makes it easier to read, without impacting clarity)

Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

Overall I think this new doc spends too many words talking about all the things that could happen in bad situations. It should focus more on what happens in good situations. Then have a section at the end that makes it clear, without giving a dozen examples, that bad casts have undefined behavior. Two examples should be enough: one with an expected JVM-style CCE failure, and one with unexpected "success".

src/library-aux/scala/Any.scala Outdated Show resolved Hide resolved
src/library-aux/scala/Any.scala Outdated Show resolved Hide resolved
src/library-aux/scala/Any.scala Outdated Show resolved Hide resolved
src/library-aux/scala/Any.scala Outdated Show resolved Hide resolved
@SethTisue
Copy link
Member

SethTisue commented Feb 22, 2024

@robstoll just wanted to say thanks for writing and updating this — we're making suggestions for adjustments, but it's super helpful to have somebody step up with that initial draft

@robstoll
Copy link
Contributor Author

robstoll commented Feb 23, 2024

@SethTisue the review is appreciated and necessary. In the end we will have hopefully a better version than the current.

I have one question though, seeing that isInstanceOf mentions union types I figured this doc is for scala 2 and scala 3. Is this correct?

@sjrd
Copy link
Member

sjrd commented Feb 23, 2024

I have one question though, seeing that isInstanceOf mentions union types I figured this doc is for scala 2 and scala 3. Is this correct?

Yes, the library, and hence the doc, is shared between Scala 2 and 3.

@robstoll
Copy link
Contributor Author

@som-snytt @Sporarum @sjrd @bishabosha @SethTisue pushed, please review again

.gitignore Outdated Show resolved Hide resolved
@He-Pin
Copy link
Contributor

He-Pin commented Feb 28, 2024

Better to add some doc about the resulting bytecode(Java) vs patten matching, as https://github.com/JakeWharton/confundus

@robstoll
Copy link
Contributor Author

@He-Pin see the first commit, I had examples of the resulting byte code (in prosa) and it was a consensus of the reviewers that those are too much detail

@robstoll robstoll force-pushed the patch-3 branch 2 times, most recently from a1178de to 45ebb10 Compare February 28, 2024 11:47
src/library-aux/scala/Any.scala Outdated Show resolved Hide resolved
src/library-aux/scala/Any.scala Outdated Show resolved Hide resolved
src/library-aux/scala/Any.scala Outdated Show resolved Hide resolved
src/library-aux/scala/Any.scala Outdated Show resolved Hide resolved
src/library-aux/scala/Any.scala Outdated Show resolved Hide resolved
Copy link

@Sporarum Sporarum left a comment

Choose a reason for hiding this comment

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

I think it's better than before, but still seems overly wordy.

I'm not super familiar with scaladoc formatting, would it be possible to use the "one sentence = one line" convention ?
If yes, I would be in favor of changing this doc to that format
(And all other doc, but this is outside this PR's scope)

src/library-aux/scala/Any.scala Outdated Show resolved Hide resolved
@robstoll
Copy link
Contributor Author

I think it's better than before, but still seems overly wordy.

I'm not super familiar with scaladoc formatting, would it be possible to use the "one sentence = one line" convention ? If yes, I would be in favor of changing this doc to that format (And all other doc, but this is outside this PR's scope)

@Sporarum me neither, do you mean start every new sentence on a new line?

@Sporarum
Copy link

Sporarum commented Feb 29, 2024

I think it's better than before, but still seems overly wordy.
I'm not super familiar with scaladoc formatting, would it be possible to use the "one sentence = one line" convention ? If yes, I would be in favor of changing this doc to that format (And all other doc, but this is outside this PR's scope)

@Sporarum me neither, do you mean start every new sentence on a new line?

Yes, doing the following for example:

-   *  On a language level it merely tells the compiler to forget any type information it has about
-   *  the receiver object and treat it as if it had type `T0`. How the compiler manages to transition from the type 
-   *  of the receiver object to `T0` is unspecified and should be considered an implementation detail of the 
-   *  corresponding compiler backend and can vary between target platform and the target version.
-   *  Transitioning to T0 might involve inserting conversions (including boxing/unboxing), casts, default values or
-   *  just no real operation at all.
+   *  On a language level it merely tells the compiler to forget any type information it has about the receiver object and treat it as if it had type `T0`.
+   * How the compiler manages to transition from the type of the receiver object to `T0` is unspecified and should be considered an implementation detail of the corresponding compiler backend and can vary between target platform and the target version.
+   *  Transitioning to T0 might involve inserting conversions (including boxing/unboxing), casts, default values or just no real operation at all.

@robstoll
Copy link
Contributor Author

I think it's better than before, but still seems overly wordy.
I'm not super familiar with scaladoc formatting, would it be possible to use the "one sentence = one line" convention ? If yes, I would be in favor of changing this doc to that format (And all other doc, but this is outside this PR's scope)

@Sporarum me neither, do you mean start every new sentence on a new line?

Yes, doing the following for example:

-   *  On a language level it merely tells the compiler to forget any type information it has about
-   *  the receiver object and treat it as if it had type `T0`. How the compiler manages to transition from the type 
-   *  of the receiver object to `T0` is unspecified and should be considered an implementation detail of the 
-   *  corresponding compiler backend and can vary between target platform and the target version.
-   *  Transitioning to T0 might involve inserting conversions (including boxing/unboxing), casts, default values or
-   *  just no real operation at all.
+   *  On a language level it merely tells the compiler to forget any type information it has about the receiver object and treat it as if it had type `T0`.
+   * How the compiler manages to transition from the type of the receiver object to `T0` is unspecified and should be considered an implementation detail of the corresponding compiler backend and can vary between target platform and the target version.
+   *  Transitioning to T0 might involve inserting conversions (including boxing/unboxing), casts, default values or just no real operation at all.

I leave that change to someone else, I think we should still adhere to 120 chars max per line

@SethTisue SethTisue self-assigned this Mar 13, 2024
@SethTisue SethTisue changed the title make it clearer that asInstanceOf is unsafe and the outcome is undefined in Scaladoc, be clearer that asInstanceOf is unsafe (and platform-dependent) Apr 8, 2024
@SethTisue
Copy link
Member

still seems overly wordy

I agree. I've pushed a commit that shortens this considerably, while still preserving what's most important, in my judgment anyway.

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 very good. Thanks all for the discussion and turning it into a concise explanation.

@SethTisue SethTisue merged commit b1917d5 into scala:2.13.x Apr 9, 2024
4 checks passed
@SethTisue
Copy link
Member

I've merged this since 2.13.14 is imminent, and since further improvements can always go in followup PRs.

But I note that we lost this:

Note that the success of a cast at runtime is modulo Scala's erasure semantics.
Therefore the expression 1.asInstanceOf[String] will throw a ClassCastException at runtime, > while the expression List(1).asInstanceOf[List[String]] will not.

The new text just talks about "erased" and "erasure" without making it really concrete like this. I guess this was because we wanted to be scrupulous about not documenting JVM-specific semantics? It might be good to add something like this back while being clear it's describing the JVM behavior specifically?

On the other hand, the Scaladoc of a particular method isn't the place to launch into long explanations of background about language semantics, what even is type erasure, and so on. But I'm not sure we have a doc page to link to that does explain that stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation No code change. Only documentation
Projects
None yet
9 participants