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

(DO NOT MERGE) don't allow identifiers to start with Lm (Letter, modifier) #34549

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

Conversation

stevengj
Copy link
Member

@stevengj stevengj commented Jan 28, 2020

As discussed in #28441, #28494, and #34507, it is a bit awkward that identifiers can start with a category-Lm (Letter, modifier) character (such as a superscript). This PR is an experiment to see how breaking it would be to change that. (You can still have Lm characters in identifiers, just not as the first character.)

(Breaks #24567.)

Can we please run PkgEval on this PR?

@stevengj stevengj added speculative Whether the change will be implemented is speculative parser Language parsing and surface syntax labels Jan 28, 2020
@KristofferC
Copy link
Sponsor Member

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

@stevengj
Copy link
Member Author

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

@KristofferC
Copy link
Sponsor Member

Wasn't it busy running the previous tests? The tests take something like 13 hours so might have to wait for quite a while to get the result of the second run.

@JeffBezanson
Copy link
Sponsor Member

Maybe should be an exception? I don't know.

@maleadt
Copy link
Member

maleadt commented Jan 28, 2020

Wasn't it busy running the previous tests? The tests take something like 13 hours so might have to wait for quite a while to get the result of the second run.

Yes, this queued two of them with no way to cancel.

[ Info: [Node 1 | 2020-01-28T09:23:12.578]: received job submission with phrase RegexMatch("@nanosoldier `runtests(ALL, vs=\":master\")`")
[ Info: [Node 1 | 2020-01-28T10:57:01.591]: received job submission with phrase RegexMatch("@nanosoldier `runtests(ALL, vs = \":master\")`")

@stevengj
Copy link
Member Author

stevengj commented Jan 28, 2020

Wasn't it busy running the previous tests?

There was an error message that the request was ill-formed or something like that, which is why I submitted another. But maybe that was from a previous attempt?

@KristofferC
Copy link
Sponsor Member

Ah, Nanosoldier benchmark bot and Nanosoldier Pkgeval bot are fighting of the same check "instance".

@StefanKarpinski
Copy link
Sponsor Member

Perhaps it would be helpful if nanosoldier posted immediately that it was doing something with a link to some kind of view of its progress?

@nanosoldier
Copy link
Collaborator

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

@DilumAluthge
Copy link
Member

Maybe should be an exception? I don't know.

That would fix some of them, but several are failing with invalid character "ˣ". Although as far as I can tell, those are always from line 154 of AbstractTensors, so it might be sufficient to fix that package.

@simeonschaub
Copy link
Member

It seems like this only affects AbstractTensors, SugarBLAS and EasyTranspose. The interesting thing to note here is that they all assign some kind of singleton to a Unicode modifier and then overload multiplication as a hack to get postfix operators. I believe, #34507 would allow for doing this in a much less hacky way by just defining the adjoint operator combined with a modifier.

@nanosoldier
Copy link
Collaborator

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

@c42f
Copy link
Member

c42f commented Feb 27, 2020

The interesting thing to note here is that they all assign some kind of singleton to a Unicode modifier and then overload multiplication as a hack to get postfix operators.

This seems a strong signal that the breakage is worthwhile. A more detailed survey of the mentioned packages:

EasyTranspose is a trivial package which just defines https://github.com/musm/EasyTranspose.jl/blob/master/src/EasyTranspose.jl

SugarBLAS also defines and uses it for things like https://github.com/lopezm94/SugarBLAS.jl/blob/master/src/SugarBLAS.jl#L274

AbstractTensors defines several postfix operators - see
https://github.com/chakravala/AbstractTensors.jl/blob/31123e0c2270fcb5e88a28c64f8cc8eaa6e28c14/src/AbstractTensors.jl#L240-L247
And tests:
https://github.com/chakravala/AbstractTensors.jl/blob/31123e0c2270fcb5e88a28c64f8cc8eaa6e28c14/test/runtests.jl#L42-L46

@JeffBezanson JeffBezanson added the triage This should be discussed on a triage call label Aug 27, 2020
@JeffBezanson JeffBezanson added this to the 2.0 milestone Aug 27, 2020
@JeffBezanson
Copy link
Sponsor Member

From triage:

  • In the abstract we think this is the right thing to do, but it's too breaking to do all of it. Consider in 2.0.
  • But, we might be able to change the parsing only of modifiers following ', so e.g. x'ᵀ becomes an operator instead of multiplication. This needs more investigation to see if such cases occur in practice. Preliminary search says probably not.

@brenhinkeller brenhinkeller added breaking This change will break code and removed triage This should be discussed on a triage call labels Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code parser Language parsing and surface syntax speculative Whether the change will be implemented is speculative
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants