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

Move unsafe operations from Expr to UnsafeExpr #8041

Merged

Conversation

@nicolasstucki
Copy link
Contributor

nicolasstucki commented Jan 20, 2020

No description provided.

@nicolasstucki nicolasstucki self-assigned this Jan 20, 2020
@nicolasstucki nicolasstucki requested a review from liufengyun Jan 20, 2020
@nicolasstucki nicolasstucki marked this pull request as ready for review Jan 20, 2020
Copy link
Contributor

liufengyun left a comment

From my experience with ScalaTest, usage of underlyingArgument is pretty common. I don't see how moving it to another package can help meta-programmers.

@nicolasstucki

This comment has been minimized.

Copy link
Contributor Author

nicolasstucki commented Jan 21, 2020

It is common to use the reflect API variant of this method, we have it all over. But this is the Expr based basEd version which has never been used in any code so far. It is important to make explicit the fact that this is the only method in the Expr API that can change normal call semantics. This implies that macro that only uses Expr guarantees that call semantics are preserved the same way inline preserves them.

@liufengyun

This comment has been minimized.

Copy link
Contributor

liufengyun commented Jan 21, 2020

If it's not used, maybe we can remove them? Meta-programming is inherently dangerous and difficult, singling out this API and putting it in an unsafe package doesn't sound like a good idea. The safety warning can be reflected in the doc of the method. For example, Option.get and List.head are unsafe, we don't move them to another place.

@nicolasstucki

This comment has been minimized.

Copy link
Contributor Author

nicolasstucki commented Jan 21, 2020

These operations may be phased out into a library later. For now it is good to keep them around to see what legitimate use cases they support. But they should not be visible out of the box to any new user.

Copy link
Contributor

liufengyun left a comment

As discussed with @nicolasstucki , the eventual goal is to remove them, this is just an intermediate step.

Make it explicit that this operation may change the call semantics.
@nicolasstucki nicolasstucki force-pushed the dotty-staging:move-unsafe-operations-out-of-expr branch from 762f361 to 9185dc4 Jan 23, 2020
@nicolasstucki nicolasstucki merged commit 7bafca1 into lampepfl:master Jan 23, 2020
2 checks passed
2 checks passed
CLA User signed CLA
Details
continuous-integration/drone/pr Build is passing
Details
@nicolasstucki nicolasstucki deleted the dotty-staging:move-unsafe-operations-out-of-expr branch Jan 23, 2020
@deusaquilus

This comment has been minimized.

Copy link

deusaquilus commented Feb 16, 2020

@nicolasstucki It looks like Expr.underlyingArgument has been moved to UnsafeExpr. I use Expr.underlyingArgument a lot for parsing quoted sections. Ever since it was removed, I started using Expr.unseal.underlyingArgument.seal everywhere. What's the rationale for removing Expr.underlyingArgument?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.