Skip to content

Do not allow named self in objects #4118

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 1 commit into from
Mar 15, 2018

Conversation

smarter
Copy link
Member

@smarter smarter commented Mar 15, 2018

object A { foo: Bla => } was already prohibited, we extend this
restriction to object A { foo => }, the latter is not really useful
and it currently causes a MatchError in SymDenotation#sourceModule when
unpickling.

@smarter smarter requested a review from nicolasstucki March 15, 2018 15:05
@odersky
Copy link
Contributor

odersky commented Mar 15, 2018

I think we should keep allowing it. The main use for named self is that you don't have to remember the qualified this syntax to access an outer this. It's inconsistent to allow this for classes but not for modules.

@smarter
Copy link
Member Author

smarter commented Mar 15, 2018

You can always refer to modules via their name so the named self doesn't seem to buy you much, but OK, I'll try to find another fix then.

@odersky
Copy link
Contributor

odersky commented Mar 15, 2018

You can always refer to modules via their name so the named self doesn't seem to buy you much, but OK, I'll try to find another fix then.

Good point! I withdraw my objection. We can drop the self type for modules then.

@smarter smarter force-pushed the fix/tasty-object-self branch 2 times, most recently from 7fd47a1 to 9e2cf04 Compare March 15, 2018 16:49
`object A { foo: Bla => }` was already prohibited, we extend this
restriction to `object A { foo => }`, the latter is not really useful
and it currently causes a MatchError in SymDenotation#sourceModule when
unpickling.
@smarter smarter force-pushed the fix/tasty-object-self branch from 9e2cf04 to ecdadc9 Compare March 15, 2018 21:35
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

Nice if a fix leads to a LOC reduction.

@smarter smarter merged commit 785c696 into scala:master Mar 15, 2018
@Blaisorblade Blaisorblade deleted the fix/tasty-object-self branch March 16, 2018 03:15
@liufengyun
Copy link
Contributor

The benchmarks for stdlib fail after this merge:

# Warmup Iteration   1: -- [E013] Syntax Error: /data/workspace/bench/tests/scala-library/Traversable.scala:92:61
92 |object Traversable extends TraversableFactory[Traversable] { self =>
   |                                                             ^^^^
   |                                       objects must not have a self type

longer explanation available when compiling with `-explain`
-- Error: /data/workspace/bench/tests/scala-library/immutable/Range.scala:156:4
156 |    validateMaxLength()
    |    ^^^^^^^^^^^^^^^^^
    |method validateMaxLength in class Range cannot be accessed as a member of Range(Range_this) from class CombinationsItr.
    | This location is in code that was inlined at /data/workspace/bench/tests/scala-library/SeqLike.scala:223
two errors found

Need to investigate more:

https://github.com/liufengyun/bench/blob/master/tests/scala-library/Traversable.scala#L92

@smarter
Copy link
Member Author

smarter commented Mar 16, 2018

This was fixed in the scala2-library submodule, you can just cherry-pick the fix: lampepfl/scala@e588767...723f222

@Blaisorblade
Copy link
Contributor

So this is a new change for Scalafix to handle? I'm not sure removing such things is worth the trouble.

@odersky
Copy link
Contributor

odersky commented Mar 19, 2018

After having played with it I now think we should keep it as it was. It is annoying that one can abstract over self by name only in classes, but not in modules. And there's no hard reason why we should disallow it.

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.

4 participants