Fix isSourceMethod

[smarter]

Review by @DarkDimius .

[odersky]

Would it not be simpler if companionMethods did not have the Method flag? Why would we need to set that flag for them?

[DarkDimius]

@smarter Does this distinction manifest itself currently?

@odersky than they would behave as fields. Wouldn't it be better to add a check for absence of Synthetic flag in isSourceMethod? Anything that is Synthetic was obviously not a user-written def-def. This would also filer out companion methods.

[odersky]

On Fri, May 1, 2015 at 10:18 PM, Dmitry Petrashko notifications@github.com
wrote:

@smarter https://github.com/smarter Does this distinction manifest
itself currently?

@odersky https://github.com/odersky than they would behave as fields.

In what sense?

Wouldn't it be better to add a check for absence of Synthetic flag in
isSourceMethod? Anything that is Synthetic was obviously not a
user-written def-def. This would also filer out companion methods.

If we look at the usages of isSourceMethod (sorry to be so vague, this was
mostly copied over from pretty old code), it seems to mean "non accessor
method". So synthetic methods would qualify as source method. For instance,
if they should print as "method", not "value".

Yes, we

—
Reply to this email directly or view it on GitHub
#519 (comment).

Martin Odersky
EPFL

[DarkDimius]

Reading once again documentation and implementation of isSourceMethod, this method puzzles me:
we have a lot of symbols which are not user defined "def" method: lazy initializers, forwarders, superaccessors, proxies in lambdalift, initial methods of Mixin. And isSourceMethod will say that many those are defined by user.

And why would ValueClasses not be applied to non-source-methods?

[smarter]

@smarter Does this distinction manifest itself currently?

No, this commit should not have any functional change, it just seemed cleaner.

Wouldn't it be better to add a check for absence of Synthetic flag in isSourceMethod?

As @odersky said, the name of the method is probably incorrect. For value classes the synthetic methods generated by SyntheticMethods like hashCode and equals should have a corresponding extension method.

[DarkDimius]

Ok, lets concentrate on what does this method do in value classes: I'd prefer to have all cases concentrated in single place: isMethodWithExtension, it would be easyer to get what it does.

So I'd say we should just inline whole body of isSourceMethod there, as isMethodWithExtension has nothing to do with method being user defined def or not, and such "reuse of similar condition" is making simple method harder to understand.

[DarkDimius]

In what sense?

Fields are handled differently by some phases, eg backend discovers fields not based on trees, but based on types. So if we'll mark companion methods as fields I'll need to filter them out there(which is not a problem, but I see no reason to remove filtering in one place to introduce in another)

[odersky]

isSourceMethod is simply a misnomer. isNonAccesorMethod looks more accurate. Then companion methods would still be "non accessor" methods, and nothing else would have to change.

[smarter]

isNonAccessorMethod is not quite right either: labels and anonymous functions are not accessors but should be excluded too, I can't think of a name that describes that.

[odersky]

Looking at the use sites of isSourceMethods, the effects of being a SourceMethod seem to be:

1. printing says "method" or "def" instead of "term" or "var".
2. It's considered for being an extension method of a value class in "isMethodWithExtension"
3. it's considered to potentially have a return statement, so it needs a result type.

It's true that companion methods should not count as source methods in that sense. So the changes LGTM.

@smarter: Maybe add a comment on isSourceMethod that distills our collective findings? If we can;t find a better name at least we can make clear what we mean by the name it has.

[DarkDimius]

Precisely because of absence of a good name, I would better separate this method in three different ones, and give them names indicating their usage.

[odersky]

I believe splitting into different ones will cause complexity elsewhere. Let's rename to isRealMethod and document what we mean by it.

[odersky]

@smarter Can you do the rename and add the comments? I'll merge once that 's in.

[smarter]

@odersky : do you consider companion$class and companion$module to be "real" methods?

[odersky]

No

On Wed, May 6, 2015 at 3:20 PM, Guillaume Martres notifications@github.com
wrote:

@odersky https://github.com/odersky : do you consider companion$class
and companion$module to be "real" methods"?

—
Reply to this email directly or view it on GitHub
#519 (comment).

Martin Odersky
EPFL

[smarter]

@odersky : I updated the PR.

[DarkDimius]

LGTM, thanks!