Skip to content

Remove NameOps implicit class (syntax extension)#8535

Closed
diesalbla wants to merge 1 commit intoscala:2.13.xfrom
diesalbla:nameops_remove
Closed

Remove NameOps implicit class (syntax extension)#8535
diesalbla wants to merge 1 commit intoscala:2.13.xfrom
diesalbla:nameops_remove

Conversation

@diesalbla
Copy link
Contributor

The NameOps implicit class is used to provide some extension methods to the Name classes and subclasses. However, because of some problem of outer references (being defined in a cake slice) they could not be turned into a plain AnyVal, so there was an allocation cost to them.

While that allocation cost was not significant (0.2%), nevertheless it is easily avoidable, by just adding some syntactic salt and vinegar to our cake.

@scala-jenkins scala-jenkins added this to the 2.13.2 milestone Nov 10, 2019
@diesalbla diesalbla changed the title Remove NameOps implicit class (syntax extension), use object Remove NameOps implicit class (syntax extension) Nov 10, 2019
@sjrd
Copy link
Member

sjrd commented Nov 10, 2019

This is going to break Scala.js.

Can't we just put these methods in Name itself? (using ThisNameType instead of T)

The NameOps implicit class is used to provide some extension
methods to the Name classes and subclasses. However, because
of some problem of outer references (being defined in a cake
slice) they could not be turned into a plain `AnyVal`, so
there was an allocation cost to them.

While that allocation cost was not significant (0.2%), it is
easily avoidable, with some syntactic salt and vinegar.
@diesalbla
Copy link
Contributor Author

diesalbla commented Nov 10, 2019

This is going to break Scala.js.

What would break Scala.js in particular? The removal of the existing implicit class (or implicit methods and extension class)? If so, perhaps they could be kept.

Can't we just put these methods in Name itself?

I do not know if those methods can be added to the Name class itself, which may be another solution. I can only assume there was a reason for not doing so.

@hrhino
Copy link
Contributor

hrhino commented Nov 11, 2019

See 724b0dc for its introduction. I think with the deprecation of the String => Name implicits it may be possible to move them. Probably worth a try.

@sjrd
Copy link
Member

sjrd commented Nov 11, 2019

What would break Scala.js in particular? The removal of the existing implicit class (or implicit methods and extension class)? If so, perhaps they could be kept.

Yes, basically the fact that it's a source-incompatible change that affects the Scala.js compiler plugin. It's nothing dramatic (the compiler breaks source compat from time to time and we have to adjust) but in this case I believe the change is a real regression in terms of API quality, so I wanted to point out that it was going to affect other codebases.

The best would be to move the methods directly on Name. Barring that, keeping the extension methods as deprecated would be good.

@hrhino
Copy link
Contributor

hrhino commented Nov 12, 2019

Oh, yes, I remember why this is needed: there's an implicit conversion String => TermName and String => TypeName, but we rely on the String => StringOps implicit conversion to call drop, take, stripSuffix, &c. on strings. Adding these methods directly to Name makes the String => *Name conversions also applicable and breaks those extension methods.

@sjrd
Copy link
Member

sjrd commented Nov 12, 2019

Haven't those String => Name conversions been deprecated for several major versions now? Could we simply remove them instead?

@diesalbla
Copy link
Contributor Author

diesalbla commented Nov 12, 2019

Haven't those String => Name conversions been deprecated for several major versions now? Could we simply remove them instead?

@sjrd Could you open a Pull Request to that effect? I would like to keep this PR on the performance concern.

@hrhino
Copy link
Contributor

hrhino commented Nov 12, 2019

I'll have a go if @sjrd doesn't have time, maybe because he's working on that thing I said I'd do and never did. The implicit conversions are part of the public API; is it within our compatibility policy to make them no longer implicit?

@diesalbla
Copy link
Contributor Author

@hrhino So, i will change the PR to avoid removing existing methods (or marking them as deprecated).

@sjrd
Copy link
Member

sjrd commented Nov 12, 2019

#8537

@retronym
Copy link
Member

#8537 has successfully performed the op to surgically excise NameOps.

@retronym retronym closed this Nov 20, 2019
@SethTisue SethTisue removed this from the 2.13.2 milestone Nov 20, 2019
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.

6 participants