Fix disabling with attributes on < 4.08#1322
Merged
emillon merged 1 commit intoocaml-ppx:masterfrom May 13, 2020
Merged
Conversation
e889dad to
dd2a87d
Compare
Collaborator
Author
|
@gpetiot let me know what you think. I tried reenabling other tests on < 4.08 in case this fixes them, but that's not the case. But maybe we can use that technique to fix tests on older versions as well. Not sure it's worth it. |
gpetiot
approved these changes
Mar 30, 2020
Collaborator
There was a problem hiding this comment.
Looks good to me. (agreed on the tests, not sure it's worth it)
As a further improvement I wonder if it would be possible to modify the mapper of Migrate_ast.Mapper to override the default Ast_mapper, especially the attribute function so we have consistent locations before 4.08. Do you think it's possible and would simplify the extend_loc_to_include_attributes function?
Collaborator
Author
|
TBH I'm not completely sure what I wonder if it would work to package the 4.10 parser (with OMP types) and use that instead of the one from compiler-libs + migrating the AST. |
Collaborator
|
Alright we can look that up in another PR, is this one ready to be merged? |
Collaborator
|
No problem with this PR, but it makes me wonder if we shouldn't just require ocaml >= 4.08. |
4826948 to
38b99d9
Compare
With OCaml < 4.08, attributes do not have a location: they only have a
name (with a loc) and a payload (with a loc). Starting with >= 4.08,
they do, as `attr_loc`.
We use OMP, so we never see the former representation. However, OMP does
not create a new loc or use `Location.none`, it copies the name's loc
into the attribute's loc.
This means that we have two situations:
- >= 4.08: parser provides precise locs:
[@@@name "payload"]
attr.attr_name.loc ^^^^
attr.attr_loc ^^^^^^^^^^^^^^^^^^^
- < 4.08: parser provides loc for attr_name, and OMP copies it:
[@@@name "payload"]
attr.attr_name.loc ^^^^
attr.attr_loc ^^^^
`Source.extend_loc_to_include_attributes`, used to disable formatting on
parts of the AST, tries to determine in which case we are to recover a
better loc in the < 4.08 case.
The existing test would check for `Location.none`, which is never the
case. So the corresponding test was disabled, but with this fix it works
fine.
Collaborator
Author
|
Yes, agree once it's easier to distribute ocamlformat independently of the current switch, >= 4.08 seems good. |
emillon
added a commit
to emillon/opam-repository
that referenced
this pull request
Aug 6, 2020
CHANGES:
#### Changes
+ Do not break inline elements such as `{i blah}` in docstrings (ocaml-ppx/ocamlformat#1346, @jberdine)
+ Distinguish hash-getter from hash-comparison infix operators. Operators of the form `#**#` or `#**.` where `**` can be 0 or more operator chars are considered getter operators and are not surrounded by spaces, as opposed to regular infix operators (ocaml-ppx/ocamlformat#1376, @gpetiot)
+ Type constraint on return type of functions is now always printed before the function body (ocaml-ppx/ocamlformat#1381, ocaml-ppx/ocamlformat#1397, @gpetiot)
#### Bug fixes
+ Restore previous functionality for pre-post extension points (ocaml-ppx/ocamlformat#1342, @jberdine)
+ Fix extra break before `function` body of a `fun` (ocaml-ppx/ocamlformat#1343, @jberdine)
Indent further args of anonymous functions (ocaml-ppx/ocamlformat#1440, @gpetiot)
+ Do not clear the emacs `*compilation*` buffer on successful reformat (ocaml-ppx/ocamlformat#1350, @jberdine)
+ Fix disabling with attributes on OCaml < 4.08 (ocaml-ppx/ocamlformat#1322, @emillon)
+ Preserve unwrapped comments by not adding artificial breaks when `wrap-comments=false` and `ocp-indent-compat=true` are set to avoid interfering with ocp-indent indentation. (ocaml-ppx/ocamlformat#1352, @gpetiot)
+ Break long literal strings at the margin (ocaml-ppx/ocamlformat#1367, @gpetiot)
+ Break after a multiline argument in an argument list (ocaml-ppx/ocamlformat#1360, @gpetiot)
+ Remove unnecessary parens around object (ocaml-ppx/ocamlformat#1379, @gpetiot)
+ Fix placement of comments on constants (ocaml-ppx/ocamlformat#1383, @gpetiot)
+ Do not escape arguments of some Odoc tags (ocaml-ppx/ocamlformat#1391, 1408, @gpetiot, @Julow)
The characters `[]{}` must not be escaped in the arguments of `@raise`, `@author`, `@version` and others.
+ Fix missing open line between multi-line let-binding with poly-typexpr (ocaml-ppx/ocamlformat#1372, @jberdine)
+ Remove trailing space after expression when followed by an attribute and break before attributes attached to multi-line phrases (ocaml-ppx/ocamlformat#1382, @gpetiot)
+ Do not add a space to minimal comments `(* *)`, `(** *)` and `(*$ *)` (ocaml-ppx/ocamlformat#1407, @gpetiot)
+ Fix attributes position in labelled arguments type (ocaml-ppx/ocamlformat#1434, @gpetiot)
+ Add missing parens around type annotation in anonymous function (ocaml-ppx/ocamlformat#1433, @gpetiot)
+ Fix alignment of 'then' keyword in parenthesised expression (ocaml-ppx/ocamlformat#1421, @gpetiot)
#### New features
+ Support quoted extensions (added in ocaml 4.11) (ocaml-ppx/ocamlformat#1405, @gpetiot)
+ Recognise eliom file extensions (ocaml-ppx/ocamlformat#1430, @jrochel)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
With OCaml < 4.08, attributes do not have a location: they only have a name (with a loc) and a payload (with a loc). Starting with >= 4.08, they do, as
attr_loc.We use OMP, so we never see the former representation. However, OMP does not create a new loc or use
Location.none, it copies the name's loc into the attribute's loc.This means that we have two situations:
>= 4.08: parser provides precise locs:< 4.08: parser provides loc for attr_name, and OMP copies it:Source.extend_loc_to_include_attributes, used to disable formatting on parts of the AST, tries to determine in which case we are to recover a better loc in the < 4.08 case.The existing test would check for
Location.none, which is never the case. So the corresponding test was disabled, but with this fix it works fine.