Extended indexing operators by Octachron · Pull Request #1064 · ocaml/ocaml

Octachron · 2017-02-24T22:28:41Z

After #69, #622 and the last developper meeting, this PR proposes to add a new family of user-definable indexing operators, that does not interfere nor can be confused with any existing indexing operators.

Extended indexing operators names start with a leading dot . followed by an operator character, except .(to avoid confusion with .. which has a different meaning ) or < (used as an opening bracket at the type level) , and a possibly empty sequence of operator characters. It must be closed with a couple of enclosing brackets ( i.e. (), [], {} ) and optionaly an assignment operator <-:

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

Expressions of the form expr_1.∘∘∘(expr_2) are then desugared to ( .∘∘∘( )) expr_1 expr_2.
Similarly for assignment, expr_1.∘∘∘(expr_2) <- expr_3 is desugared to ( .∘∘∘( )<-) expr_1 expr_2 expr_3. The same syntactic sugar is applied to all bracket variants (i.e. {, (, [ ).

	Expression	Desugaring
Access	expr_1.∘∘∘⟨expr_2⟩	( .∘∘∘⟨ ⟩ ) expr_1 expr_2
Assignment	expr_1.∘∘∘⟨expr_2⟩ <- expr_3	( .∘∘∘⟨ ⟩ <- ) expr_1 expr_2 expr_3

The compulsory operator character should help to mark these operators as user-defined, and decrease the confusion potential of these operators. Moreover, the use of a qualified dot respects the use of unqualified . as a projection primitive.

gasche · 2017-02-24T22:36:12Z

I like it! .$?$(...) is terribly ugly, but .~[ ] and .%( ) both make sense, and I agree it's a good way out of your conflict with type-directed resolution.

(In general I also like the idea of having operator names mirror the operator syntax, similar to Agda's mixfixes, which has first been given to me, in an OCaml contet, by Nicolas Pouillard in 2010. But this was already part of your previous proposal.)

gasche · 2017-02-24T22:37:56Z

If I understand correctly, the testsuite currently checks the parsed AST. Would it be possible to also test that these programs are pretty-printed back correctly (that is, in a way that can be parsed back to the same thing)?

Octachron · 2017-02-24T22:41:08Z

The parsetree/source.ml test already checks that the pretty-printed ast can be parsed back.

Drup · 2017-02-25T18:32:30Z

parsing/parser.mly

+        mkexp @@ Pexp_apply(id , [Nolabel, $1; Nolabel, $4; Nolabel, $7]) }
+  | simple_expr DOTOP LBRACE expr RBRACE
+      { let id = mkexp @@ Pexp_ident( ghloc @@ Lident ("." ^ $2 ^ "{}")) in
+        mkexp @@ Pexp_apply(id, [Nolabel, $1; Nolabel, $4]) }


This is orthogonal, but it might be beneficial to add rules of the form simple_expr DOTOP LBRACE expr error to give better error messages.

Good point, fixed.

Drup · 2017-02-25T18:44:27Z

The patch looks good to me (and I like the proposed solution).

I wonder if maybe it would be time to change the representation of (non module) identifier to a sum type. That would make the printing implementation slightly less ridiculous.

Octachron · 2017-02-25T22:52:22Z

Extending on @Drup comment, I have added a comment to clarify the cryptic string manipulation in parsing/pprintast.ml which extract operator informations from its name.

damiendoligez · 2017-03-07T12:33:36Z

Looks good but I don't much like the restrictions on characters:

for < : is there a real reason to disallow it (i.e. a conflict in the grammar) ?
for . : of course .. is a keyword, but I don't think that's a reason to disallow all "operators" that begin with ...

Octachron · 2017-03-07T18:54:10Z

With the current scheme (i.e. using a new DOTOP token in the lexer) allowing either . or < as starting characters would require modification in unrelated part of the parser due to the use of < and > as brackets for object type:

For <, in let f: 'a .< x:'a> -> 'a= … , .< will be read as a DOTOP(<) token.
For ., in let f: <x:'a; ..> -> 'a = …, ..> will be read as a DOTOP(.>) token.

There is potentially two ways to get around this limitation: restrict the syntax to merge .%( and friends in a single token rather than two (.% () (a solution that was suggested to me by @Drup earlier) or generalize the syntax by removing the lexer modification and implementing it as a grammar modification only.

The first solution will slightly break the symmetry between classical indexing operators and the extended ones: a . (b) will be valid, but not a .% (b). The last solution will allow more whitespace at strange positions: a .. . .. % (b) .

damiendoligez · 2017-03-29T07:11:17Z

OK, those are good reasons.

This commit adds extended indexing operators to the parser. For instance, ``` let (.%()) p (x,y) = p.( x ).( y ) ;; p.%(0,0) ``` Extended indexing operators name starts with a leading dot "." followed by an operator symbol, except "." or "<" and a sequence of any operators and must be closes with a couple of enclosing parentheses ( i.e. "()", "[]", "{}" ) and then an optional assignment operator "<-": * '.' dotsymbolchar symbolchar* '(' ')' ['<-'] * '.' dotsymbolchar symbolchar* '[' ']' ['<-'] * '.' dotsymbolchar symbolchar* '{' '}' ['<-'] Similarly, expressions of the form * expr_1 '.' dotsymbolchar symbolchar* '(' expr_2 ')' * expr_1 '.' dotsymbolchar symbolchar* '[' expr_2 ']' * expr_1 '.' dotsymbolchar symbolchar* '{' expr_2 '}' and * expr_1 '.' dotsymbolchar symbolchar* '(' expr_2 ')' '<-' expr_3 * expr_1 '.' dotsymbolchar symbolchar* '[' expr_2 ']' '<-' expr_3 * expr_1 '.' dotsymbolchar symbolchar* '{' expr_2 '}' '<-' expr_3 are desugared to * ('.' dotsymbolchar symbolchar* '(' ')' ) expr_1 expr_2 * ('.' dotsymbolchar symbolchar* '[' ']' ) expr_1 expr_2 * ('.' dotsymbolchar symbolchar* '{' '}' ) expr_1 expr_2 and * ('.' dotsymbolchar symbolchar* '(' ')' '<-' ) expr_1 expr_2 expr_3 * ('.' dotsymbolchar symbolchar* '[' ']' ) expr_1 expr_2 expr_3 * ('.' dotsymbolchar symbolchar* '{' '}' '<-' ) expr_1 expr_2 expr_3

Octachron · 2017-09-12T20:53:25Z

What is the status of this PR for 4.0{6,7}?

If the restriction on the operators character is deemed too complicated, a coarser and simpler restriction may be to forbid the characters <> with the motivation that they are too close typographically from the left and right angle brackets ⟨⟩ and thus confusing in for instance x.<?>.(y).

damiendoligez · 2017-09-15T12:17:34Z

I think the restriction is OK. It will always be possible to write unreadable programs so we shouldn't spend too much time on this "issue".

Please rebase and I'll merge.

lpw25 · 2017-09-15T12:26:59Z

I just noticed that this doesn't support things like x.Foo.%(...) -- analogous to x.Foo.label. This is a shame. I guess its too late to get that in before the freeze. Maybe Damien will generously consider fixing this omission a bug fix?

mshinwell · 2017-09-15T13:30:46Z

I took the liberty of rebasing this myself. Will be ready to merge once Travis etc finishes.

damiendoligez · 2017-09-15T15:48:04Z

Maybe Damien will generously consider fixing this omission a bug fix?

Depending on the complexity of the fix and the proximity of the release, maybe.

Octachron · 2017-09-15T21:34:15Z

@lpw25 , I will send a PR adding back qualified operators tomorrow morning.

nojb · 2017-09-25T08:19:56Z

Changes

+  user-defined indexing operators, obtained by adding at least
+  one operator character after the dot symbol to the standard indexing
+  operators: e,g ".%()", ".?[]", ".@{}<-"
+  (Florian Angeletti, review by Damien Doligez and Gabriel Radanne)


@Octachron I think this PR breaks backwards compatibility in some edge cases: see https://caml.inria.fr/mantis/view.php?id=7637.

yallop · 2017-10-19T14:28:05Z

Not necessarily a problem, but worth noting here: this change uses up the syntactic space that was considered for the succinct version of overriding local open, i.e.

M.!(e)

as a shorthand for

let open! M in e

With this PR, M.!(e) is instead interpreted as (.!()) M e.

Drup · 2017-10-19T14:39:11Z

@yallop You mean, the one that was rejected (#218) ? :)

yallop · 2018-10-13T19:29:10Z

Another issue with this change (which I should have spotted earlier): it reserves .~, which has been used by MetaOCaml for splicing for a long time.

However, it appears that none of the code available via OPAM is yet using .~, so it's not too late to restore compatibility. I'll open a new pull request with more details (update: #2106).

) I've also regenerated the test to be sure that printing now works (in the one very simple example). In the long run: * The primitives should probably all be made lowercase for simplicity and for consistency with the IR (currently the "constructor-like" ones get initial caps). * We should be testing fexpr printing as well as parsing (roundtrip test).

Co-authored-by: cuihtlauac <cuihtlauac@users.noreply.github.com>

gasche added the caml-weekly-news label Feb 24, 2017

gasche approved these changes Feb 24, 2017

View reviewed changes

Drup reviewed Feb 25, 2017

View reviewed changes

This was referenced Mar 7, 2017

Simplify the definition and use of custom index operators #69

Closed

User-defined indexing operator without array indexing #622

Closed

Octachron force-pushed the ocaml-indexop-last branch from b0dd657 to 03f8294 Compare September 12, 2017 20:41

Octachron added 6 commits September 12, 2017 22:51

Manual for extended indexing operators

2339a78

GPR#1064: extended indexing operators changes

ced4886

GPR#1064: add a comment to pprintast acrobatics

01eac7e

GPR#1064: add unclosed bracket error to DOTOP

a570784

Updated changes

4aec17f

Octachron force-pushed the ocaml-indexop-last branch from 03f8294 to 4aec17f Compare September 12, 2017 20:52

Merge with trunk

91ba07e

mshinwell added the consider-for-release label Sep 15, 2017

damiendoligez merged commit 201ee17 into ocaml:trunk Sep 15, 2017

Octachron mentioned this pull request Sep 16, 2017

Qualified extended indexing operators #1347

Merged

nojb reviewed Sep 25, 2017

View reviewed changes

xclerc mentioned this pull request Oct 3, 2017

Tweak grammar for extended indexing operators #1392

Merged

yallop mentioned this pull request Oct 13, 2018

Disallow .~ in extended indexing operators. #2106

Merged

vicuna mentioned this pull request Mar 14, 2019

Breaking change in parser (probably related to GPR#1064 - Extended indexing operators) #7637

Closed

gpetiot mentioned this pull request Aug 4, 2021

Support for old versions of the compiler. ocaml-ppx/ocamlformat#633

Closed

EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024

[create-pull-request] automated change (ocaml#1064)

420b420

Co-authored-by: cuihtlauac <cuihtlauac@users.noreply.github.com>

Conversation

Octachron commented Feb 24, 2017

Uh oh!

gasche commented Feb 24, 2017

Uh oh!

gasche commented Feb 24, 2017

Uh oh!

Octachron commented Feb 24, 2017

Uh oh!

Drup Feb 25, 2017

Choose a reason for hiding this comment

Uh oh!

Octachron Feb 25, 2017

Choose a reason for hiding this comment

Uh oh!

Drup commented Feb 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Octachron commented Feb 25, 2017

Uh oh!

damiendoligez commented Mar 7, 2017

Uh oh!

Octachron commented Mar 7, 2017

Uh oh!

damiendoligez commented Mar 29, 2017

Uh oh!

Octachron commented Sep 12, 2017

Uh oh!

damiendoligez commented Sep 15, 2017

Uh oh!

lpw25 commented Sep 15, 2017

Uh oh!

mshinwell commented Sep 15, 2017

Uh oh!

damiendoligez commented Sep 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Octachron commented Sep 15, 2017

Uh oh!

nojb Sep 25, 2017

Choose a reason for hiding this comment

Uh oh!

yallop commented Oct 19, 2017

Uh oh!

Drup commented Oct 19, 2017

Uh oh!

yallop commented Oct 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Drup commented Feb 25, 2017 •

edited

Loading

damiendoligez commented Sep 15, 2017 •

edited

Loading

yallop commented Oct 13, 2018 •

edited

Loading