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

unspecialcase unicode escapes [WIP] #6661

Closed
wants to merge 1 commit into from

Conversation

martijnhoekstra
Copy link
Contributor

Explore removing the special status of unicode escapes.

In this PR, unicode escapes are treated like any other escape, except

  • In triple quoted strings, unicode escapes are expanded, and a deprecation warning is issued
  • In raw interpolations unicode escapes are expanded, and a deprecation warning is issued

@scala-jenkins scala-jenkins added this to the 2.13.0-M5 milestone May 17, 2018
@martijnhoekstra
Copy link
Contributor Author

If CI is green, I'd like to request a community build to see what the fallout of such a change is, if any.

@martijnhoekstra
Copy link
Contributor Author

Fixes scala/bug#3220

@SethTisue SethTisue added the WIP label May 17, 2018
@martijnhoekstra
Copy link
Contributor Author

I would like to see community build for this PR, just to be sure, before I start polishing things up (right now there is a bunch of repeated code that needs cleaning up)

But 2.13.x community build is AFAIK not really that far along. Is there an issue for that that I can watch?

@som-snytt
Copy link
Contributor

Why isn't the change just dependent on -Xno-uescape? Or, since the point is to default that to true, you could also rename it something sane like, -Xunicode-escapes.

It's not obvious to me that the escape is great for interpolation.

@lrytz
Copy link
Member

lrytz commented May 31, 2018

The 2.13 community build is currently not doing much because most projects don't compile yet for 2.13 (new collections, scala/community-build#710). You could however prepare a PR against 2.12.x with the change, then we can run the 2.12 community build.

@martijnhoekstra
Copy link
Contributor Author

martijnhoekstra commented Jun 1, 2018

@som-snytt I knew -Xno-uescape existed in the back of my mind, but it escaped my attention. I'll look further in to that. That said, at least for now, I remain convinced that handling unicode escapes differently from other escapes, and special-casing that different handling for string and char literals, interpolations and comments brings a lot more complexity than justified. I may have to justify my position on that further.

@martijnhoekstra
Copy link
Contributor Author

@lrytz porting to 2.12 for a community build sounds like a good idea. A different dummy PR for that is probably going to be easier than re-targeting this PR, and then re-targeting it back. Does that strike you as a reasonable course of action?

@lrytz
Copy link
Member

lrytz commented Jun 1, 2018

Yes!

martijnhoekstra pushed a commit to martijnhoekstra/scala that referenced this pull request Jun 2, 2018
haphazard cherry-pick against 2.12.x

This is a a re-target of scala#6661 against 2.12 with the intention
of running a community build to see what the fallout is
of such a change if any.
martijnhoekstra pushed a commit to martijnhoekstra/scala that referenced this pull request Jun 2, 2018
haphazard cherry-pick against 2.12.x

This is a a re-target of scala#6661 against 2.12 with the intention
of running a community build to see what the fallout is
of such a change if any.
@adriaanm adriaanm removed this from the 2.13.0-M5 milestone Aug 3, 2018
@adriaanm
Copy link
Contributor

adriaanm commented Aug 3, 2018

Unscheduling from M5 since this is WIP.

@adriaanm adriaanm added this to the 2.13.x milestone Aug 3, 2018
@SethTisue
Copy link
Member

@martijnhoekstra interested in carrying on with this...?

@martijnhoekstra
Copy link
Contributor Author

martijnhoekstra commented Aug 10, 2018

@SethTisue yes, once we can get a sizable community build going for 2.13. Some things could break, and if I carry on with this, I'd need to have a robust migration story.

Not sure whether this could realistically be targeted for a minor release, so this might have to go for 2.14 or 3

@SethTisue
Copy link
Member

closing for inactivity, can be revived & reopened later.

the 2.13 community build currently only has about ~60 (out of ~190) projects, it'll probably won't be much larger until around the time RC1 comes out

@SethTisue
Copy link
Member

I assume this will also resolve scala/bug#10873

@SethTisue
Copy link
Member

I put scala/bug#3220 on the RC1 milestone, to help us not forget

@SethTisue SethTisue removed this from the 2.13.1 milestone Feb 3, 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
6 participants