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

Disallow .~ in extended indexing operators. #2106

Merged
merged 5 commits into from Oct 22, 2018

Conversation

Projects
None yet
5 participants
@yallop
Copy link
Member

commented Oct 13, 2018

PR #1064 introduced support for user-defined indexing operators. The syntax is similar to the built-in operators (.(), .[], etc.) but with symbol characters between the dot and the delimiters, like this:

let (  .%( ) ) p (x,y) = p.(x).(y)
let (  .$?${ } ) p (x,y,z) = p.(x).(y).(z)
let (  .~[ ] <- ) p (x,y,z,t) v = p.(x).(y).(z).(t) <- v 

Unfortunately, allowing the sequence .~ is incompatible with MetaOCaml's longstanding splice syntax:

.~expression

Fortunately, there doesn't appear to be any code actually using .~ yet, so it's not too late to restore compatibility. The following table shows which user-defined indexing operators are currently in use by OCaml packages available via OPAM:

! $ % & ? @
owl !{} $() ${} %() %{} ?()
labltk ![] ![]
lablgtk ![]
reed-solomon %() %[] %{} %{} &{} &{}
orec %{}
phantom-algebra %[] %[] %() %()
pyml !$[] ![] %$[] %[] &() @$() @()
ocaml testsuite %() %() %[] %{} ?[] @() @[] @{}
ocamlformat testsuite %() %() %() %[] %[] %{} %{} ?[] @() @[] @{}

This PR removes ~ from the set of characters allowed in user-defined indexing operators, eliminating the incompatibility with MetaOCaml's syntax.

@gasche

This comment has been minimized.

Copy link
Member

commented Oct 13, 2018

(cc @Octachron)

@Octachron

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2018

I am wondering if it would make sense to add .~ as a currently unused lexical token in order to reserve its use for MetaOcaml?

@yallop

This comment has been minimized.

Copy link
Member Author

commented Oct 13, 2018

That's an interesting idea. 5122818 makes .~ a reserved sequence.

@yallop

This comment has been minimized.

Copy link
Member Author

commented Oct 16, 2018

@Octachron, @gasche: would either of you like to review this? (I realise that it needs a Changes entry.)

@gasche

gasche approved these changes Oct 18, 2018

Copy link
Member

left a comment

The implementation looks fine.

The feature looks very reasonable to me: it restricts a very-new feature that has apparently not been used in the wild yet (although it is part of a release OCaml version), I think the cost/benefit is reasonable.

@Octachron, extended operators are your expertise, so I will wait (not merge) to give you an opportunity to comment here. If you are against the change, I may have to reconsider.

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

You should document .~ as a keyword in the manual (chapter 7.1, lexical conventions).

@Octachron
Copy link
Contributor

left a comment

I also think that the change is reasonable.

Another option might have be to just emit a DOTTILDE token if the dotop operator is.~ but removing ~ from the list of starting character for DOTOP is simpler. Moreover, it let MetaOcaml decides how to handle cases like .~!x .

Concerning the reserved error mechanism in the lexer, I have few remarks in term of explicitness, but it seems nice enough. (Initially, I was thinking of just importing the DOTTILDE token and the relevant lexer definition from the MetaOCaml patch; but the current implementation makes for a nicer error message. )

Show resolved Hide resolved parsing/lexer.mll Outdated
@yallop

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2018

@damiendoligez:

You should document .~ as a keyword in the manual (chapter 7.1, lexical conventions).

Done (02cf280)

@gasche

gasche approved these changes Oct 19, 2018

Copy link
Member

left a comment

This looks good to me if the CI passes.

@yallop yallop force-pushed the yallop:dot-tilde branch from 0691225 to 629ee03 Oct 19, 2018

@yallop

This comment has been minimized.

Copy link
Member Author

commented Oct 22, 2018

The check-typo CI failure:

./manual/manual/refman/lex.etex:1.1: [missing-header] bad copyright header (first line)

looks spurious, since none of the etex files have copyright headers.

@dra27

This comment has been minimized.

Copy link
Contributor

commented Oct 22, 2018

This might be because of the recent changes to check-typo?

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Oct 22, 2018

check-typo gets called explicitly on manual/manual/refman/lex.etex without regard for the typo.prune attribute on manual. The failure is completely unrelated to this PR, so I'm merging now.

@damiendoligez damiendoligez merged commit 20f4c6c into ocaml:trunk Oct 22, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@gasche

This comment has been minimized.

Copy link
Member

commented Oct 22, 2018

check-typo: This is a problem that is fixed in #1905 (still waiting for a review), and was noticed by @Armael who fixed it in an ad-hoc way for his own PR #2906 (I missed it). I just pushed 2713258 in trunk to fix it for good.

@yallop yallop deleted the yallop:dot-tilde branch Oct 22, 2018

damiendoligez added a commit to damiendoligez/ocaml that referenced this pull request Nov 5, 2018

Disallow .~ in extended indexing operators. (ocaml#2106)
Disallow .~ in the lexer to preserve MetaOCaml compatibility.

314eter added a commit to 314eter/tree-sitter-ocaml that referenced this pull request Jan 15, 2019

314eter added a commit to 314eter/tree-sitter-ocaml that referenced this pull request Jan 19, 2019

maxbrunsfeld added a commit to tree-sitter/tree-sitter-ocaml that referenced this pull request Feb 7, 2019

Support new features (#22)
* Fix problems with character literals in comments

* Do not parse unclosed comments and quoted strings

* Update known failures

* Only allow line number directives with filename (ocaml/ocaml#931)

* Rename dot_operator to indexing_operator

* Disallow .~ in indexing operator (ocaml/ocaml#2106)

* Add test for indexing operator

* Support empty variants (ocaml/ocaml#1546)

* Support binding operators (ocaml/ocaml#1947)

* Use tree-sitter 0.14.0

* Cleanup trvis config
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.