Skip to content

Enchancement #3008 #3228

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

Conversation

CucumisSativus
Copy link
Contributor

Refined printer checks if showAsInflisAnotType annotation is used (issue #3008).

I would be grateful if you could lend me a hand in testing my solution, couldn't find any tests for RefinedPrinter.

If something is missing please let me know, i'm happy to improve this PR :)

Copy link
Member

@dottybot dottybot left a comment

Choose a reason for hiding this comment

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

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Have an awesome day! ☀️

@smarter
Copy link
Member

smarter commented Oct 1, 2017

Hi! This already looks pretty good, a few improvements that could be made:

@smarter
Copy link
Member

smarter commented Oct 1, 2017

By the way, to check that the modified test works correctly, you can run just the REPL printing tests by typing in sbt:

dotty-compiler/test-only -- --tests=typePrinterTests

@@ -126,8 +126,8 @@ class RefinedPrinter(_ctx: Context) extends PlainPrinter(_ctx) {
def isInfixType(tp: Type): Boolean = tp match {
case AppliedType(tycon, args) =>
args.length == 2 &&
!Character.isUnicodeIdentifierStart(tycon.typeSymbol.name.toString.head)
// TODO: Once we use the 2.12 stdlib, also check the @showAsInfix annotation
!Character.isUnicodeIdentifierStart(tycon.typeSymbol.name.toString.head) &&
Copy link
Member

Choose a reason for hiding this comment

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

This should be an || not an && as we want infix types to be used either for type constructors which start with a symbol or that have the @showAsInfix annotation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's one more problem. I didnt notice that this annotation takes a boolean argument, which should be checked as well.

@CucumisSativus
Copy link
Contributor Author

@smarter Thanks for pointing me to tests, looks like there's a problem - not found: type showAsInfix (even after import). I'll take a closer looks at this

@smarter
Copy link
Member

smarter commented Oct 1, 2017

Ah you're running into a known bug (#2462), you can work around it by using the fully qualified name of the annotation e.g:

scala> @scala.annotation.showAsInfix type Mappy[T, U] = Map[T, U]

@CucumisSativus CucumisSativus force-pushed the show_as_inflix_used_in_refined_printer branch 2 times, most recently from d5b83d8 to 1a2ad9c Compare October 2, 2017 19:13
Refined printer checks if shoAsInflixAnotType annotation is used.
Based on "SI-4700 Make infix notation default for symbolic types" from
original scala repository
@CucumisSativus CucumisSativus force-pushed the show_as_inflix_used_in_refined_printer branch from 1a2ad9c to 5d70d1d Compare October 2, 2017 19:16
@CucumisSativus
Copy link
Contributor Author

@smarter I added tests, the only problem I have right now is using type alias type Mappy[T,U] = Map[T,U], when accessing aliased type whenever i call
tycon.classSymbol or tycon.typeSymbol i receive Map not Mappy, do you know how could I access Mappy there?

@smarter
Copy link
Member

smarter commented Oct 2, 2017

Hmm this is probably the Dotty typechecker being too eager to dealias things, I'll look into it but it might be tricky to fix. We can merge this PR without the type aliases working since it already improves things anyway.

@CucumisSativus
Copy link
Contributor Author

@smarter so my proposition is to merge this pull request and open a new issue where someone (maybe even me, with some guidance :) ) can check how to approach type aliases

@smarter
Copy link
Member

smarter commented Oct 3, 2017

LGTM, thanks for your contribution!

@smarter smarter merged commit 432ceac into scala:master Oct 3, 2017
@smarter
Copy link
Member

smarter commented Oct 3, 2017

maybe even me, with some guidance :)

I'm afraid this might be a hard issue to fix :) I suggest you take a look at https://github.com/lampepfl/dotty/issues?q=is%3Aissue+is%3Aopen+label%3Aexp%3Anovice or ask on http://gitter.im/lampepfl/dotty if you're looking for other things to contribute.

@CucumisSativus
Copy link
Contributor Author

CucumisSativus commented Oct 3, 2017

LGTM, thanks for your contribution!

Thanks for your guidance and help :)

@CucumisSativus CucumisSativus deleted the show_as_inflix_used_in_refined_printer branch October 3, 2017 17:05
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