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

ExplicitResultTypes: backquote types when needed #1578

Merged
merged 5 commits into from
Apr 4, 2022

Conversation

danicheg
Copy link
Contributor

@danicheg danicheg commented Mar 27, 2022

This fixes #1219. I was struggled with a working fix for all type-symbols but didn't succeed. I'd be glad to get help in moving this onward.

@bjaglin
Copy link
Collaborator

bjaglin commented Mar 27, 2022

Thanks for the PR, that's already a step forward! I didn't have a look at it yet, but can you maybe elaborate what challenges you faced with other type symbols? You can open a draft PR with failing tests as a starting point, it's always valuable for others to pick up where you left off.

@mlachkar
Copy link
Collaborator

Hello!
thanks for your contribution.
This code should not be outside of the method g.shortType(preProcessed, history), because this method is recursive.
There is a test example that would break with your fix:

object ExplicitResultTypesBackQuote {
  case class `Foo-Bar`(i: Int)
}

object TestBackQuote {
  val foobar = ExplicitResultTypesBackQuote.`Foo-Bar`(42)
}

it would output:

object TestBackQuote {
  val foobar: `ExplicitResultTypesBackQuote.Foo-Bar` = ExplicitResultTypesBackQuote.`Foo-Bar`(42)

}

It's tempting to fix the issue, by taking the shortType and pattern match on it, but it can't work, because you need to do that recursively as it's done in the method def shortType. You can check this PR, where we discussed how to fix issues on this rule #1495

If you have any question, don't hesitate!

@danicheg
Copy link
Contributor Author

@mlachkar Thanks for the review. I moved impl to ScalafixGlobal#shortType, so now traits and nested case classes/traits support backticks.

@danicheg danicheg changed the title Fix ExplicitResultTypes output in case of backticks for case classes Fix ExplicitResultTypes output in case of backticks for case classes/traits Mar 28, 2022
@danicheg danicheg changed the title Fix ExplicitResultTypes output in case of backticks for case classes/traits Fix ExplicitResultTypes output in case of backticks for case classes / traits Mar 28, 2022
Copy link
Collaborator

@bjaglin bjaglin left a comment

Choose a reason for hiding this comment

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

Thanks for the iteration, it's shaping up! Don't forget to run ./bin/scalafmt.

@danicheg
Copy link
Contributor Author

Thank you for your review, @bjaglin!

@bjaglin bjaglin changed the title Fix ExplicitResultTypes output in case of backticks for case classes / traits ExplicitResultTypes` : backquote types when needed Apr 3, 2022
@danicheg danicheg changed the title ExplicitResultTypes` : backquote types when needed ExplicitResultTypes: backquote types when needed Apr 3, 2022
@bjaglin bjaglin requested a review from mlachkar April 3, 2022 16:56
@bjaglin bjaglin merged commit fe183d0 into scalacenter:main Apr 4, 2022
@danicheg danicheg deleted the issue-#1219 branch April 4, 2022 10:25
@tgodzik
Copy link
Contributor

tgodzik commented Apr 14, 2022

Thanks for the great work @danicheg ! I managed to backport the changes to Metals also https://github.com/scalameta/metals/pull/3829/files

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.

ExplicitResultTypes should backquote identifiers if necessary
4 participants