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

RFC: parse a' as call expression #33683

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

simeonschaub
Copy link
Member

@simeonschaub simeonschaub commented Oct 26, 2019

It seems weird to me that the adjoint operator still gets its own expression, because except for its special syntax it is basically a regular function call, so I thought, I'd just give it a go. This should also simplify some meta programming, since ' doesn't have to be special-cased anymore. What do people think, is this a good idea?

@simeonschaub simeonschaub changed the title RFC: parse :(q') as call expression RFC: parse :(a') as call expression Oct 26, 2019
@yuyichao yuyichao added the kind:breaking This change will break code label Oct 26, 2019
@JeffBezanson JeffBezanson changed the title RFC: parse :(a') as call expression RFC: parse a' as call expression Oct 28, 2019
@JeffBezanson
Copy link
Sponsor Member

Tough call. It does seem like this would have been better, but I'm not sure it's worth changing.

@StefanKarpinski
Copy link
Sponsor Member

This is technically breaking since macros may be relying on the current AST representation.

@simeonschaub
Copy link
Member Author

I totally understand the concerns, and that this might be too breaking. On the other hand I wonder, how much code really relies on this, and if this wouldn't actually fix more potential bugs than it causes problems. 😁
(The section in the manual is quite outdated and doesn't even mention ', so one could make the argument, that it's an undocumented feature. :trollface:)

@StefanKarpinski
Copy link
Sponsor Member

True, that's fair. We do make technically breaking changes if they don't actually break packages.

@StefanKarpinski StefanKarpinski added kind:minor change Marginal behavior change acceptable for a minor release kind:speculative Whether the change will be implemented is speculative compiler:lowering Syntax lowering (compiler front end, 2nd stage) labels Oct 28, 2019
@MasonProtter
Copy link
Contributor

MasonProtter commented Jan 24, 2020

Just bumping this thread. Has anyone formed any strong opinions on this in the past few months? I think it's a good idea. It's more consistent with how we treat prefix / infix operators and it makes it possible to do things like locally shadow ''s definition and make a new one without type piracy.

@jessebett
Copy link
Contributor

The type piracy here is coming from AD packages wanting to use f' to represent, e.g., the Zygote.adjoint(f) for reverse-mode derivative function of f.

@simeonschaub
Copy link
Member Author

Would it make sense to run PkgEval on this, to get a sense of how breaking this actually is?

@maleadt
Copy link
Member

maleadt commented Jan 29, 2020

@nanosoldier runtests(ALL, vs = ":master")

@JeffBezanson
Copy link
Sponsor Member

Good news, everybody. It's not necessary to change parsing to do this. It can be done in lowering instead:

   '|'|  (lambda (e) (expand-forms `(call (top adjoint) ,(cadr e))))

(julia-syntax.scm)

Currently, all syntax without an identifier name (e.g. indexing, vcat, etc.) lowers to call some Base function. We have discussed before changing them all to call lexically-scoped names like __getindex__, __adjoint__, etc., where Base exports default definitions const __getindex__ = getindex etc. Then you can lexically shadow those strange operators. Maybe it's time to just do this?

@MasonProtter
Copy link
Contributor

Is lowering to a :call expression necessary to get #34507 ?

@JeffBezanson
Copy link
Sponsor Member

If I understand correctly, #34507 is a pure parsing issue --- entirely about surface syntax. If we did it, we'd probably parse suffixed adjoint operators as call expressions. Yes, that would be a bit inconsistent with ' by itself being an expression head, but backwards compatibility is like that sometimes.

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here. cc @maleadt

@simeonschaub
Copy link
Member Author

I think this could be worth reconsidering for a 2.0 release. Would it make sense to add it to the milestone?

@simeonschaub simeonschaub added this to the 2.0 milestone Oct 30, 2020
@simeonschaub simeonschaub added parser Language parsing and surface syntax and removed compiler:lowering Syntax lowering (compiler front end, 2nd stage) kind:minor change Marginal behavior change acceptable for a minor release labels Oct 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:breaking This change will break code kind:speculative Whether the change will be implemented is speculative parser Language parsing and surface syntax
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants