Make def generated from givens not synthetic

[odersky]

Fixes #12949

[odersky]

The problem is how to know whether some method is a structural given instance or
an implicit conversion associated with an implicit class. Previously we characterized
these methods by their Synthetic flag, but that meant that they were not automatically
exported. So we now drop Synthetic and use a flag GivenClass on the class half of
the pair.

The old scheme is still recognized in order to maintain Tasty backwards compatibility.
Tasty forwards compatibility is lost, that's why we need to wait for the next minor release.

A better fix would be to give the synthetic class of a structural given instance a special semantic name.
That would also get rid of the weirdness that the generated class of a given instance like

given foo: C with ...

is visible under the name foo. But that would break backwards Tasty compatibility, so we have to wait
until we can implement that.

[soronpo]

The old scheme is still recognized in order to maintain Tasty backwards compatibility.
Tasty forwards compatibility is lost, that's why we need to wait for the next minor release.

So this means this PR will be merged only from 3.1?

[odersky]

@soronpo Yes, but I think we'll move to 3.1 soon anyway.

[soronpo]

@soronpo Yes, but I think we'll move to 3.1 soon anyway.

Then I better get to finding more compatibility braking bugs soon 😉

[soronpo]

@soronpo Yes, but I think we'll move to 3.1 soon anyway.

@odersky
Then I propose creating a warning message for 3.0.x notifying more refined "given with" types are not supported yet, if such structures are used.

[odersky]

@soronpo Not sure exactly what the warning should do and who would design and implement it.

[soronpo]

Not sure exactly what the warning should do

Currently (until 3.1) given ... with ... type T structures are not valid. They exist in the language, but the compiler does not support them fully. This leads to an error that the user needs to figure out, possibly spend several hour minimizing, possibly find this issue, and possibly gets frustrated. Deprecating this feature until 3.1 is the way to go, IMO. I don't mind falling into the pit myself. I know Scala 3 is not yet completely charted territory and I tread lightly. I just want to prevent others from falling into the same pit without even putting a warning sign in front of it.

and who would design and implement it.

I don't mind helping out, but unfortunately currently my table is full. I'm trying my best minimizing and documenting the issues I find as I'm migrating my library to Scala 3.

[odersky]

Currently (until 3.1) given ... with ... type T structures are not valid.

That's a bit extremist. What does not work is wildcard exports for them. They are nevertheless widely used, so introducing a blanket warning would be extremely annoying. If someone can find a targeted warning that is clearly not annoying then I am happy to see a PR. But personally i think it will be very difficult to achieve and not worth the effort.

[smarter]

Instead of dropping Given and adding a new GivenClass, what prevents us from keeping Given?

[bishabosha]

I think this can be done without the need for new flags and TASTy changes, unless there is a good reason for the new flag

[bishabosha]

I think this should not be merged without a minor TASTy bump, it breaks the assumption that a type can not have Given or Implicit flag. Unless we say that implementations should expect any possible flag

[odersky]

I think this should not be merged without a minor TASTy bump, it breaks the assumption that a type can not have Given or Implicit flag. Unless we say that implementations should expect any possible flag

I think flags are sufficiently underspecified that we don't need to worry about this. The TreeUnpickler works unchanged for old as well as new version. That should be our test for version bumps. If an old TreeUnpickler works with the new format, we are good.

[smarter]

If an old TreeUnpickler works with the new format, we are good.

That's good enough for the compiler but not necessarily for macro authors, who might have to adjust their macros if the semantics of tasty change, but anyway it seems that we'll bump to 3.1.0-RC1 (and therefore bump tasty) before the release.

[bishabosha]

At present the Scala 2 TASTy reader scans flags not compatible with Scala 2 to see if it needs to special case certain definitions, (such as Invisible) - so currently it explicitly checks for certain "redundant" flags it can safely ignore, such as Opaque on a class. So if a definition suddenly had a new flag that is not known to be safe to ignore, then the user will get a formatted error. Perhaps this approach was wrong.

TLDR: this will break Scala 2 TASTy reader for previously accepted code

[odersky]

OK, we have this under next minor release anyway.

[bishabosha]

this needs a rebase

[odersky]

@bishabosha Can you check that the semanticdb expect files are now correct? Thanks!

[bishabosha]

@odersky please may you check the addition I made, (I added a test for a symbol being a structural given instance method)

otherwise I think the semanticdb changes are good

We will not explore the alternative proposed in this review

[bishabosha]

@odersky I have updated so that in semanticdb the type parameter in the structural given is recorded as belonging to the summoner method

[bishabosha]

So to summarise the point about the semanticdb changes:
We will go with showing a definition occurrence for the type parameter of the class in structural givens, as this gives the most consistent way to rename the type parameter with an IDE

[dwijnand]

@dos65 are you able to finish reviewing this today?

[dos65]

@dwijnand @bishabosha

Now, It looks good from my side.
I haven't dived in code details, only checked how it affects semanticdb output.