PR#6604: Only allow directives with filename and at the beginning of the line by tadeuzagallo · Pull Request #931 · ocaml/ocaml

tadeuzagallo · 2016-11-20T13:49:54Z

This updates the lexer to enforce that directives have a filename and are placed at the beginning of the line, as suggested in the comments in the Mantis PR referred to in the title.

I had to update one existing test, and added 2 more to verify the new intended behaviour.

bobzhang · 2016-11-20T17:50:03Z

would you add a test case for the first line?

# 1 "xx.ml" (* this should still be fine *)

tadeuzagallo · 2016-11-20T18:31:08Z

Added it. Also included a couple more directives to test the non-first line case and two consecutive directives (which was the reason I had to add the code to "put back" the newline at the end of a directive).

dra27 · 2016-11-20T19:34:31Z

Could you add an entry to the Changes file (worth heading it with both "PR#6604, GPR#931")?

tadeuzagallo · 2016-11-20T20:01:24Z

Oops, forgot about it, sorry. I was unsure under which section to add it, so I just added to "Bug fixes", let me know if that's not the best place for it. I'm also not sure whether the name of the reporter is correct, from Mantis I can only see the username, yet everyone seems to use real names in the changelog.

gasche · 2016-11-20T20:08:42Z

Regarding the Changelog:

You should use a bullet-point of the form * rather than -, as this change can break working programs.
Entries should be sorted by MPR number first, then by GPR number. As all other entries in this section have no MPR number, your entry should go first.

Regarding the patch itself: the position handling is non-trivial and would need a careful review. In particular, we should make sure that the change does not affect the lexing positions of other tokens (than the lexer directives) in the source.

tadeuzagallo · 2016-11-20T20:16:12Z

I've updated the changelog, thanks.

With regards to the position handling, my thinking was that: as long as the Invalid_directive errors were reported correctly it'd be correct, as the directive will override the position. Would that be correct?

dra27 · 2016-11-20T20:21:00Z

Agreed - I've been staring at the position manipulation code for a while, trying to convince myself it's definitely correct!

xavierleroy · 2016-12-04T15:20:31Z

I'm not a big fan of pushing back already-read data by tweaking fields of a lexbuf. Is there no other way?

I was not expecting to have an error if a directive is recognized not at beginning of line. It would feel more natural to me to just analyze it as if it was not a directive, but instead # followed by a number, etc.

tadeuzagallo · 2016-12-04T22:04:15Z

Thanks for the feedback. I think I found a cleaner solution, the only problem is that the "line out of bounds" errors will be reported as starting on column 1 rather than 0, as the hash was matched first. Not sure if this is a problem though.

mshinwell · 2017-08-10T07:28:57Z

@tadeuzagallo Is this ready for review? If so please rebase.

damiendoligez · 2017-08-21T12:03:10Z

It's really not a problem that the error report is off by 1 character.

xavierleroy

This looks good to me and could be merged as is. I wonder if a slightly simpler implementation is possible, see comment in code.

xavierleroy · 2017-10-03T18:31:20Z

parsing/lexer.mll

+      { update_loc lexbuf None 1 false 1;
+        try directive lexbuf
+        with Failure(_) -> HASH
      }


This looks reasonable, but is there any chance of unifying these two cases? Something like

if not (at_beginning_of_line lexbuf.lex_start_p) then HASH else try directive lexbuf with Failure _ -> HASH

where, tentatively,

let at_beginning_of_line pos = (pos.pos_cnum = pos.pos_bol)

Just a suggestion: I didn't try and I'm not sure it would work.

This worked perfectly, thanks.

…the line

…wline skipped

tadeuzagallo · 2017-10-03T21:33:07Z

Sorry for the delay, I have rebase and updated the code with the suggestions.

dra27 · 2017-10-04T08:31:40Z

Looks good to me

@gasche/@damiendoligez - is this definitely OK for 4.06?

gasche · 2017-10-04T08:53:26Z

I appreciate the change, but I'm not convinced that it is totally without risk, and I think the issue it is fixing can tolerate a delay. (I think the cost of a potential regression noticed only after the second beta offset the gains.) I would personally vote for inclusion (now) in trunk, but not in the release branch.

dra27 · 2017-10-04T09:01:52Z

@gasche - that was my feeling too. So let's remove it from the milestone but merge to trunk now

gasche · 2017-10-04T09:06:22Z

Thanks! And thanks again @tadeuzagallo.

xavierleroy · 2017-10-07T12:04:41Z

Since this PR was merged the CI at Inria is broken but only under OpenBSD (i386). The breakage is in ocamldoc/:

../boot/ocamlrun ../ocamlc -nostdlib -I ../stdlib -pp 'sh ./remove_DEBUG' -I ../parsing -I ../utils -I ../typing -I ../driver -I ../bytecomp -I ../toplevel -I ../stdlib -I ../compilerlibs -I ../otherlibs/str -I ../otherlibs/dynlink -I ../otherlibs/unix -I ../otherlibs/graph -absname -w +a-4-9-41-42-44-45-48 -warn-error A -safe-string -strict-sequence -strict-formats -bin-annot -c odoc_text_parser.ml
File "/home/ci/workspace/main/flambda/false/slave/ocaml-openbsd-32/ocamldoc/odoc_text_parser.mly", line 27, characters 31-33:
Error: Syntax error

Probably a bad interaction with the remove_DEBUG script and OpenBSD's implementation of sed. To be investigated.

…the line (#931)

xavierleroy · 2017-10-07T13:44:01Z

This is what the file looks like after sed preprocessing:

(* DEBUG statement removed *)# 82 "odoc_text_parser.ml"
let yytransl_const = [|

xavierleroy · 2017-10-07T13:56:25Z

Plausible fix in [trunk 801993d] . Waiting for a round of CI to confirm.

xavierleroy · 2017-10-07T15:54:00Z

CI looks happy.

dra27 · 2017-10-07T16:18:48Z

Excellent - though unexpected, it's sort of evidence that it's working, right?

Before long, that script needs to be changed anyway - when we start building OCaml using the Linux subsystem on Windows, using scripts with -pp will become "non-portable" behaviour, so we'll probabaly want to preprocess the files manually.

* 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

reflects the change from ocaml#931 / ocaml#6604

* Add -basic-block-sections compiler flag. Requires compiler configured with function sections. * Don't elide labels and jumps to fallthrough with basic_block_section * Move code earlier in emit.mlp * Emit section name for each Llabel and handle size and cfi directives * Assign blocks to sections * Don't emit jumps for fallthroughs between blocks in the same section * Add section_name field to Llabel * Emit section names associated with labels instead of emitting each basic block label in its own section * Don't need to pass -function-sections to enable basic block sections * Fix bug: Don't use labelled_insn_end.label = -1 * Rework section names to allow "." suffix * Add field Linear.fundecl.fun_section_name * Emit function or basic block section name correctly for fun entry * Entry label of cfg should be unique, add CR but validator tests fail and I'm not sure if 1 is explicitly used elsewhere, so keep it for now. * Add CR regarding checking sections of Cfg_with_layout as part of Cfg_equivalence. * Format * Flag for -basic-block-sections should be separate from -ocamlcfg * Improve comment * cross_section with dst = labelled_insn_end.label is not an error * Fix corner case label handling * Use Label.equal * Format

* Fix removing defined width on swiper's images (ocaml#836) * Fix disable swiper's auto resize (ocaml#836)

tadeuzagallo force-pushed the pr6604 branch from b8ca15f to 3f8400c Compare November 20, 2016 20:12

tadeuzagallo force-pushed the pr6604 branch from 5007b8a to 79a8eb2 Compare November 20, 2016 20:58

tadeuzagallo force-pushed the pr6604 branch from 3177c95 to a09625a Compare December 4, 2016 21:58

damiendoligez added this to the 4.06.0 milestone Sep 29, 2017

xavierleroy reviewed Oct 3, 2017

View reviewed changes

tadeuzagallo added 7 commits October 3, 2017 23:02

PR#6604: Only allow directives with filename and at the beginning of …

1415804

…the line

Add extra test for valid directives

9b6624b

Update changelog

e6bfb5a

Fix position manipulation - add 1 char to lex_start_p.bol from the ne…

d9d3afb

…wline skipped

Avoid manipulating lexbuf directly

1c7dcfb

Update tests

f6921ad

Rebase and simplify lexer changes based on feedback

7bf6d5d

tadeuzagallo force-pushed the pr6604 branch from a09625a to 7bf6d5d Compare October 3, 2017 21:31

dra27 removed this from the 4.06.0 milestone Oct 4, 2017

dra27 merged commit 74ca5ee into ocaml:trunk Oct 4, 2017

xavierleroy pushed a commit that referenced this pull request Oct 7, 2017

PR#6604: Only allow directives with filename and at the beginning of …

0e484e2

…the line (#931)

hhugo mentioned this pull request Jan 25, 2018

Filenames are now mandatory for line directives ocaml-community/cppo#51

Closed

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

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

98c29fd

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

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

b9d6fb0

This was referenced Mar 14, 2019

Re: Installing O'Caml 3.0 under MPW #2435

Closed

Expressions like 3#0;; are ignored by the toplevel and the compiler #6604

Closed

4.07.0+beta2: doesn't recognize line directives #7777

Closed

oandrieu added a commit to oandrieu/ocaml that referenced this pull request Feb 11, 2021

update the manual concerning line number directives

3370545

reflects the change from ocaml#931 / ocaml#6604

oandrieu mentioned this pull request Feb 11, 2021

update the manual concerning line number directives #10215

Merged

garrigue pushed a commit to garrigue/ocaml that referenced this pull request Mar 3, 2021

update the manual concerning line number directives

884ffaa

reflects the change from ocaml#931 / ocaml#6604

smuenzel pushed a commit to smuenzel/ocaml that referenced this pull request Mar 30, 2021

update the manual concerning line number directives

a9e589c

reflects the change from ocaml#931 / ocaml#6604

dra27 mentioned this pull request Nov 14, 2022

Allow line directives without filename #11723

Closed

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

Fix editor swiper's width (ocaml#931)

9b0d5b3

* Fix removing defined width on swiper's images (ocaml#836) * Fix disable swiper's auto resize (ocaml#836)

Conversation

tadeuzagallo commented Nov 20, 2016 • edited by gasche Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bobzhang commented Nov 20, 2016

Uh oh!

tadeuzagallo commented Nov 20, 2016

Uh oh!

dra27 commented Nov 20, 2016

Uh oh!

tadeuzagallo commented Nov 20, 2016

Uh oh!

gasche commented Nov 20, 2016

Uh oh!

tadeuzagallo commented Nov 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dra27 commented Nov 20, 2016

Uh oh!

xavierleroy commented Dec 4, 2016

Uh oh!

tadeuzagallo commented Dec 4, 2016

Uh oh!

mshinwell commented Aug 10, 2017

Uh oh!

damiendoligez commented Aug 21, 2017

Uh oh!

xavierleroy left a comment

Choose a reason for hiding this comment

Uh oh!

xavierleroy Oct 3, 2017

Choose a reason for hiding this comment

Uh oh!

tadeuzagallo Oct 3, 2017

Choose a reason for hiding this comment

Uh oh!

tadeuzagallo commented Oct 3, 2017

Uh oh!

dra27 commented Oct 4, 2017

Uh oh!

gasche commented Oct 4, 2017

Uh oh!

dra27 commented Oct 4, 2017

Uh oh!

gasche commented Oct 4, 2017

Uh oh!

xavierleroy commented Oct 7, 2017

Uh oh!

xavierleroy commented Oct 7, 2017

Uh oh!

xavierleroy commented Oct 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xavierleroy commented Oct 7, 2017

Uh oh!

dra27 commented Oct 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

tadeuzagallo commented Nov 20, 2016 •

edited by gasche

Loading

tadeuzagallo commented Nov 20, 2016 •

edited

Loading

xavierleroy commented Oct 7, 2017 •

edited

Loading