Skip to content

Add syntax for qualified types #8

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

Merged
merged 3 commits into from
Nov 11, 2024
Merged

Add syntax for qualified types #8

merged 3 commits into from
Nov 11, 2024

Conversation

mbovel
Copy link
Owner

@mbovel mbovel commented Sep 13, 2024

This is a port of @Sporarum's work with the following changes:

  • Now using an intermediate untyped tree QualifiedTypeTree that is desugared into an annotation during Typer instead of creating the annotation during parsing already. This is more in-line with how similar features are implemented and is preferred by IDEs developers according to Martin.
  • Dropped support for the alternative trailing lambda syntax; only kept the set notation one.
  • Dropped support for indentation-based qualified types. They can now only be surrounded by curly braces.

@mbovel
Copy link
Owner Author

mbovel commented Sep 15, 2024

@@ -158,6 +160,10 @@ object Feature:
if ctx.run != null then ctx.run.nn.ccEnabledSomewhere
else enabledBySetting(captureChecking)

/** Is qualifiedTypes enabled for this compilation unit? */
def qualifiedTypesEnabled(using Context) =
enabledBySetting(qualifiedTypes)

Choose a reason for hiding this comment

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

This only checks if the feature is enabled by a setting. I see that you also added object qualifiedTypes in language.scala. We probably want to do enabled(qualifiedTypes) then.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We cannot isolate Qualified Types to one scope, as they might escape in a number of ways
Therefore it only makes sense to be enabled or disabled overall

This is similar to what is done for capture types

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ah right, now I remember, thanks @Sporarum. So we should probably do like for capture checking, and I should remove object qualifiedTypes.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Oh, and now I remember why I needed object qualifiedTypes: it is used to know that the qualifiedTypes feature is experimental. If I remove it, I get the following error:

-- Error: tests/printing/qualifiers.scala:26:9 ---------------------------------
26 |  val x: Int with x > 0 = 1
   |         ^
   |       class qualified is marked @experimental
   |
   |       Experimental definition may only be used under experimental mode:
   |         1. in a definition marked as @experimental, or
   |         2. an experimental feature is imported at the package level, or
   |         3. compiling with the -experimental compiler flag.

Copy link
Owner Author

Choose a reason for hiding this comment

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

The logic that does that is in Feature.experimentalAutoEnableFeatures I believe.

Choose a reason for hiding this comment

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

Ok, that makes sense. Thanks for explaining.

@@ -156,6 +156,9 @@ object untpd extends Trees.Instance[Untyped] with UntypedTreeInfo {
*/
case class CapturesAndResult(refs: List[Tree], parent: Tree)(implicit @constructorOnly src: SourceFile) extends TypTree

/** { x: T with p }  (only relevant under qualifiedTypes) */

Choose a reason for hiding this comment

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

Could you make the example use the case class parameters? and possibly explain what a missing paramName means?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Done: 9d5a4fa.

@@ -65,6 +66,7 @@ object Feature:
(clauseInterleaving, "Enable clause interleaving"),
(pureFunctions, "Enable pure functions for capture checking"),
(captureChecking, "Enable experimental capture checking"),
(qualifiedTypes, "Enable experimental qualified types"),

Choose a reason for hiding this comment

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

We probably don't need to say "experimental" here, it might change at some point 😅

Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems to be the standard for experimental features, I see no reason to deviate

(And when the time comes, we can change description)

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, as for many things, we just copied what was done for capture checking, one line above in this case. I also think keeping experimental for now is probably okay.

Choose a reason for hiding this comment

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

Ok, sure. I just looked at the other experimental features and only two of them explicitly say that they are experimental, so it seemed to deviate from the standard. Still, it's not a strong opinion and doesn't really matter.

@@ -2034,6 +2071,8 @@ object Parsers {
atSpan(in.offset) {
makeTupleOrParens(inParensWithCommas(argTypes(namedOK = false, wildOK = true, tupleOK = true)))
}
else if in.token == LBRACE && followingIsQualifiedType() then

Choose a reason for hiding this comment

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

Can we merge parsing qualified types and refinements and get rid of the lookahead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I tried, but couldn't manage to, but that doesn't mean it's impossible

Copy link
Owner Author

Choose a reason for hiding this comment

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

Good point.

I just made a test trying with a single lookahead to see if the next character is an IDENT and it seems to work. Maybe we needed an additional lookahead (':') only for the lambda syntax? I'll further test my change.

Copy link
Owner Author

@mbovel mbovel Oct 17, 2024

Choose a reason for hiding this comment

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

It seems to work: 23114ba.

Choose a reason for hiding this comment

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

Nice!

@mbovel mbovel force-pushed the mb/qualifiers branch 2 times, most recently from cb7be48 to 23114ba Compare October 17, 2024 15:14
@mbovel
Copy link
Owner Author

mbovel commented Oct 18, 2024

@mbovel mbovel requested a review from KacperFKorban October 18, 2024 12:27
Copy link

@KacperFKorban KacperFKorban left a comment

Choose a reason for hiding this comment

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

Looks good!

@mbovel mbovel merged commit b521df1 into qualifiers Nov 11, 2024
@mbovel mbovel deleted the mb/qualifiers branch November 11, 2024 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants