Skip to content

Fix #5498: error for postfixOps if language feature not enabled #8388

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Mar 28, 2020
Merged

Fix #5498: error for postfixOps if language feature not enabled #8388

merged 3 commits into from
Mar 28, 2020

Conversation

robstoll
Copy link
Contributor

@robstoll robstoll commented Feb 26, 2020 •
edited
Loading

resolves #5498. It seems like dotty does not output a warning any more (see for instance: https://scastie.scala-lang.org/L8epbiXgQryCYHRWgbx69g)
I saw that PostFixOp is de-sugared in Desugar and hence not available in Typer (where I would have put the check). That's also the reason why I have put the check in Desugar

Following things to consider:

  1. it's my first PR to the compiler
  2. the neg-test fails (putting the same test into pos fails as expected, so I am not sure what's wrong there) => Edit: I found my mistake, I improved the error messages in tests so that others will not fall over this problem
  3. there are build failures due the new check, so it seems to work - shall I enable the feature or rewrite the code? => Edit: I enabled postfixOps for dotty.

@robstoll robstoll marked this pull request as ready for review February 26, 2020 17:08
@som-snytt som-snytt mentioned this pull request Feb 26, 2020
Merged
@robstoll robstoll mentioned this pull request Feb 28, 2020
Closed
@robstoll robstoll force-pushed the #5498-postFix-error branch from 24590a3 to 584a0f0 Compare March 12, 2020 07:58
@robstoll
Copy link
Contributor Author

I realised that the current implementation in Desugar might be controversial due to function application. For instance:

xs filterNot (ys contains)

contains is considered as postfixOp, I guess it is only detected within select that this is a function application and therefore actually an infix application and not a postfix application.

Thoughts?

@robstoll robstoll force-pushed the #5498-postFix-error branch from 584a0f0 to 4ec009d Compare March 12, 2020 13:46
@robstoll robstoll changed the title WIP: error for postfixOps if language feature not enabled Fix #5498: error for postfixOps if language feature not enabled Mar 12, 2020
@robstoll
Copy link
Contributor Author

The community build fails due to the new error. How should we proceed on this one?

@robstoll robstoll mentioned this pull request Mar 12, 2020
Merged
@OlivierBlanvillain
Copy link
Contributor

OlivierBlanvillain commented Mar 12, 2020 •
edited
Loading

Looking at the log the error seams to be coming from scalap. If your PR requires changes in that project you should first make a PR againts this branch with the requiered changes to scalap, then update the submodule in the current PR to get everything green!

@robstoll
Copy link
Contributor Author

robstoll commented Mar 12, 2020 •
edited
Loading

@OlivierBlanvillain that's what I tried to do with dotty-staging/scala#3, please review and merge 🙂

I guess there will be more errors, I'll fix one by one and ask for help if I am stuck.

@robstoll robstoll force-pushed the #5498-postFix-error branch 2 times, most recently from 69fd043 to 0fb199b Compare March 24, 2020 21:17
@smarter
Copy link
Member

smarter commented Mar 24, 2020

contains is considered as postfixOp, I guess it is only detected within select that this is a function application and therefore actually an infix application and not a postfix application.

I think it makes sense to require postfixOps to write this code, and it also matches what scalac does.

smarter
smarter reviewed Mar 24, 2020
View reviewed changes
compiler/src/dotty/tools/dotc/ast/Desugar.scala Outdated
@@ -1675,6 +1675,9 @@ object desugar {
}
else {
assert(ctx.mode.isExpr || ctx.reporter.errorsReported || ctx.mode.is(Mode.Interactive), ctx.mode)
if (!ctx.featureEnabled(nme.postfixOps)) {
ctx.error(s"Usage of postfix operator `${op.name}`\n add `import scala.language.postfixOps` or enable the compiler flag `-language:postfixOps`", t.sourcePos)
Copy link
Member

Choose a reason for hiding this comment

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

Can you make the message matches the one used by scalac ?

Copy link
Contributor Author

@robstoll robstoll Mar 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Like this?

postfix operator ${op.name} needs to be enabled
by making the implicit value scala.language.postfixOps visible.
----
This can be achieved by adding the import clause 'import scala.language.postfixOps'
or by setting the compiler option -language:postfixOps.
See the Scaladoc for value scala.language.postfixOps for a discussion
why the feature needs to be explicitly enabled.

Copy link
Member

Choose a reason for hiding this comment

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

sure

Copy link
Contributor Author

@robstoll robstoll Mar 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

done, I think wrapping the operator name with `` is an improvement though, so I keep this if you don't mind.

@robstoll robstoll force-pushed the #5498-postFix-error branch 3 times, most recently from 67fcc6b to 5f21014 Compare March 24, 2020 22:30
smarter
smarter approved these changes Mar 26, 2020
View reviewed changes
Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

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

LGTM!

robstoll and others added 3 commits March 26, 2020 20:08
@robstoll
error when postfix and postfixOps not enabled
79fa5a4 
moreover:
- activate postfixOps for dotty
- improve error message in CompilationTest in case one
  forgot to define `// error`
- include errors in case expected or row number does not match
- add add/withoutLanguageFeature to TestFlags
@robstoll
update scalap to latest version, including fix for postfixOps
8943894
@robstoll
use ctx.echo instead of println in TypeComparer.showGoal
6a65c8e
@robstoll robstoll force-pushed the #5498-postFix-error branch from 5f21014 to 6a65c8e Compare March 26, 2020 19:08
@smarter smarter merged commit 3d17b02 into scala:master Mar 28, 2020
@robstoll robstoll deleted the #5498-postFix-error branch May 1, 2020 05:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smarter smarter smarter approved these changes

Assignees

@smarter smarter

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Follow Scala 2.13's lead and make postfixOps syntax an error (not just a warning) unless the feature is explicitly enabled
3 participants
@robstoll @OlivierBlanvillain @smarter