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

[REVISIT] Dogfood value discard [ci: last-only] #10476

Closed
wants to merge 28 commits into from

Conversation

som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Jul 20, 2023

Inspired by Seth's branch. #10091

There were one or two actual bugs (Imports.scala, NodePrinters.scala), a few improvements in expression. One test that didn't test anything.

@scala-jenkins scala-jenkins added this to the 2.13.13 milestone Jul 20, 2023
@som-snytt som-snytt force-pushed the dogfood/value-discard branch 2 times, most recently from 43a230a to 759a50b Compare July 29, 2023 18:28
@som-snytt
Copy link
Contributor Author

som-snytt commented Jul 29, 2023

synchronized { f(); () } is verbose idiom, or synchronized[thing.type] { thing }.

One-legged if needs polish. Cannot just add unit value because that breaks tailrec.

Given a choice of API, use the Unit-valued method. (update not put, etc) There is no good alternative to stack.pop().

@som-snytt
Copy link
Contributor Author

hopefully this is internal only

ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.NewVectorIterator.drop")
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.NewVectorIterator.take")

@som-snytt som-snytt force-pushed the dogfood/value-discard branch 2 times, most recently from dd1c5d6 to 9917ce3 Compare August 2, 2023 22:48
@som-snytt
Copy link
Contributor Author

oh, actual warnings. It doesn't care because all it wants is a string.

[error] /home/jenkins/workspace/scala-2.13.x-validate-main/src/reflect/scala/reflect/internal/Positions.scala:146:18: a type was inferred to be `Object`; this may indicate a programming error.
[error]     val source = if (tree.pos.isDefined) tree.pos.source else ""
[error]                  ^
[error] /home/jenkins/workspace/scala-2.13.x-validate-main/src/reflect/scala/reflect/internal/Types.scala:3770:31: a type was inferred to be `Any`; this may indicate a programming error.
[error]     private def levelString = if (settings.explaintypes.value) level else ""
[error]                               ^
[error] two errors found

@som-snytt
Copy link
Contributor Author

The idea was that def f(sb: StringBuilder): sb.type; f(new StringBuilder) does not use the value outside the side effect, so it warns that the result is abandoned.

Then val sb = new StringBuilder; f(sb) also does not warn, though the value is not "more used".

The desired analysis: a value was not used (as an input) to produce another value. (Out of scope here.)

In that context, is it helpful (at all) to warn about f(x.g), where x.g may or may not be expensive or idempotent?

It would be less noisy to treat f the same as any Unit-valued method.

@som-snytt
Copy link
Contributor Author

I was sampling some fresh dogfood, and just to spice it up, tweaked "unibranch if" under -Xsource:3, so I was also trying -Xsource:3migration.

The override inference deserves refinement, as previously noted. By extraordinary coincidence, someone complained on twitter yesterday.

@lrytz It doesn't faithfully infer the overridden signature (faithful to dotty behavior). To recap, dotty abandons "concrete overrides abstract" rule, and also infers a type from overridden symbols (plural). The puzzling example was that I had to annotate coreBTypes (the class structure includes links between bTypes and its coreBTypes member). I could not explain (from partial knowledge) why dotty compiles it. In dotty, BTypes was subtly altered in recent backport.

https://github.com/scala/scala/blob/2.13.x/src/compiler/scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala#L36

https://github.com/scala/scala/blob/2.13.x/src/compiler/scala/tools/nsc/backend/jvm/BTypes.scala#L39

https://github.com/lampepfl/dotty/blob/main/compiler/src/dotty/tools/backend/jvm/BTypesFromSymbols.scala#L34

https://github.com/lampepfl/dotty/blob/main/compiler/src/dotty/tools/backend/jvm/BTypes.scala#L39

@som-snytt
Copy link
Contributor Author

I don't remember if there were any great lessons from the exercise, but it's clear that this is too heavily annotated (or the warning is too noisy for this code base). Glad to reopen if conditions improve.

@som-snytt som-snytt closed this Jan 13, 2024
@som-snytt som-snytt mentioned this pull request Jan 13, 2024
@SethTisue SethTisue removed this from the 2.13.14 milestone Jan 16, 2024
@som-snytt som-snytt changed the title Dogfood value discard [ci: last-only] [REVISIT] Dogfood value discard [ci: last-only] Jan 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants