Skip to content

private[this] inference does not work for constructor parameters #22620

Open
@yaniskas

Description

@yaniskas

Compiler version

3.6.3

Minimized code

import scala.collection.mutable.ArrayBuffer

class PrivateTest[-M](private val v: ArrayBuffer[M])

Output

-- Error: .\PrivateTest.scala:3:34 ---------------------------------------------
3 |class PrivateTest[-M](private val v: ArrayBuffer[M])
  |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |contravariant type M occurs in invariant position in type scala.collection.mutable.ArrayBuffer[M] of value v

Expectation

This code compiles correctly if you use the deprecated private[this] syntax, so according to the Scala 3 reference, it looks like the compiler should treat v as if it had been declared private[this], and the code should compile without errors. It also happens to compile correctly if you do not include the private val declaration at all and let the compiler infer it.

Activity

som-snytt

som-snytt commented on Feb 17, 2025

@som-snytt
Contributor

Apparently, that is an intentional feature:
https://github.com/scala/scala3/blob/3.6.3/compiler/src/dotty/tools/dotc/core/SymDenotations.scala#L2644

It would be nice if the reference clarified that.

The idea is that deciding if a field is required, etc, is already done "by construction" in constructors for class params, so adding val signals that you really want it.

Inferring it "back" to private local means the field can be eliminated, which is not desirable.

A possible nuance would be "don't eliminate inferred private local, only use that for variance checking", but maybe that is too much nuance.

This corner case is served just by introducing a private member "manually".

yaniskas

yaniskas commented on Feb 17, 2025

@yaniskas
Author

Actually, I feel like that nuance you mentioned would make more sense. I would say that all the old uses of private[this] should be served by just marking things as private.

Also, while the val case is served by omitting the val declaration, if you want to make the field a var you have no other option than to use the manual method you mentioned where you pass a constructor parameter with a different name and then initialize a var field with that value. Isn't this too much boilerplate to replace a deprecated feature?

yaniskas

yaniskas commented on Feb 17, 2025

@yaniskas
Author

And there's another related issue: if you use the -rewrite option in scalac on a working example with a private[this] constructor parameter, it produces uncompilable code

som-snytt

som-snytt commented on Feb 17, 2025

@som-snytt
Contributor

@yaniskas thanks, I'll also check out the rewrite bug. I submitted a PR only to document status quo.

You could open a "Discussion" topic if you feel strongly about the feature. Or I could "convert to discussion" this ticket; my PR doesn't have to "fix" it.

I don't have an opinion. I observe the sometimes-gnarly syntax is "just a convenient shorthand" that is optimized for the common case. var is less common, and caring about fields is less common. Also variance. Or private constructors.

Actually, I do have an opinion: I'd prefer the simplicity of private meaning the same thing, without exemption.

Edit: I misunderstood what you meant about the rewrite: not that it produces bad syntax, just that it requires refactoring in this case. That's interesting, whether it should refuse the rewrite.

yaniskas

yaniskas commented on Feb 17, 2025

@yaniskas
Author

With the rewrite I meant that it just removes the [this] part, and then fails the variance check. In my opinion, the rewrite is doing the correct thing, and it's the variance check that should pass.

To me it just seems like a regression/unnecessary removal of a feature. And I don't see why the syntax for val declarations should be simpler than for var declarations in this case. I guess it would be nice to convert it to a discussion topic, but will it become less visible that way?

som-snytt

som-snytt commented on Feb 17, 2025

@som-snytt
Contributor

I'll leave it here with the triage label so it's not overlooked.

added
area:varianceIssues related to covariance & contravariance.
and removed
stat:needs triageEvery issue needs to have an "area" and "itype" label
on Feb 18, 2025
Gedochao

Gedochao commented on Feb 18, 2025

@Gedochao
Contributor

To me it just seems like a regression/unnecessary removal of a feature.

I wouldn't treat this a regression, although I agree that this bug is causing -rewrite to produce erroneous code on Scala 3.4+, so it could be argued it is one (which in my book bumps its priority by a notch).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

    Participants

    @som-snytt@Gedochao@KacperFKorban@yaniskas

    Issue actions

      private[this] inference does not work for constructor parameters · Issue #22620 · scala/scala3