Skip to content

Ast_mapper.PpxContext: store type-checking related flags#1921

Merged
gasche merged 2 commits intoocaml:trunkfrom
gasche:type-flags-in-ppx-context
Jul 21, 2018
Merged

Ast_mapper.PpxContext: store type-checking related flags#1921
gasche merged 2 commits intoocaml:trunkfrom
gasche:type-flags-in-ppx-context

Conversation

@gasche
Copy link
Copy Markdown
Member

@gasche gasche commented Jul 20, 2018

It is a bug of the current PpxContext that -rectypes is not passed as
part of the context: any ppx extension that would like to be able to
load .cmi files in the same initial environment as the user code would
break because loading -rectypes-using .cmi is disallowed if
Clflags.recursive_types is not set.

(I found this issue while debugging a ppx_import user that compiles
with -rectypes -- ocaml-ppx/ppx_import#25 )

I tried to add all the other Clflags that seem related to
type-checking and might cause a program to not-type-check if they
are not correctly passed:

  • recursive_types
  • principal
  • transparent_modules
  • unboxed_types
  • unsafe_string

Drup
Drup approved these changes Jul 20, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@Drup Drup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. The patch looks good.

@xclerc
Copy link
Copy Markdown
Contributor

xclerc commented Jul 20, 2018

I think it might be useful to also store whether float arrays
are flat or not.

@gasche
Copy link
Copy Markdown
Member Author

gasche commented Jul 20, 2018

Config.flat_float_array is a configuration constant decided at configure-time, it cannot be "restored" (set) by the ppx context: either the ppx binary was compiled in the same configuration as the host, or you are fried.

@xclerc
Copy link
Copy Markdown
Contributor

xclerc commented Jul 20, 2018

I am slightly confused: is there a check that the invoked ppx
rewriter agrees flat-float-array-wise with the process that
produced the AST?

@gasche gasche mentioned this pull request Jul 20, 2018
Merged
@gasche
Copy link
Copy Markdown
Member Author

gasche commented Jul 20, 2018

As far as I can tell, there is no such thing today. But it is also not clear how to implement it. When restoring the context inside the ppx binary (the API for that is fixed by Ast_mapper.PpxContext), what do you do if the float-arrayness does not match? You fail with an error? (That sounds reasonable, but note that possibly you otherwise would have processed the file perfectly.) This is non-trivial design question, I don't care about flat-float-array for the bug that I am fixing, so I would rather avoid the concern overload and keep that out of the PR.

@let-def
Copy link
Copy Markdown
Contributor

let-def commented Jul 20, 2018 •
edited
Loading

I wonder what you have in mind for flat-float-array, I can imagine that if you are sharing the same ppx binary with different version of the compilers it might be useful.
Unmarshalling float arrays is unsafe in this context, though I don't think AST contains such thing.

@xclerc
Copy link
Copy Markdown
Contributor

xclerc commented Jul 20, 2018

Well, the thing is if you do not fail explicitly you are accepting
the possibility of a segfault down the line. The AST does not
contain float arrays as of today, but this property is not guaranteed
to hold tomorrow, and I don't think the assumption is commented.

If the information is stored in the context and #1589 is merged
at some point, you will be able to detect the mismatch.

@let-def
Copy link
Copy Markdown
Contributor

let-def commented Jul 20, 2018

What about changing the magic numbers depending on flat-float-array setting?
I think this issue is outside the scope of this PR.

@gasche gasche force-pushed the type-flags-in-ppx-context branch from cda0df0 to 08c48a5 Compare July 20, 2018 11:30
@gasche
Copy link
Copy Markdown
Member Author

gasche commented Jul 20, 2018

(I just rebased to add a Changes entry.)

gasche added 2 commits July 20, 2018 13:38
@gasche
PpxContext: store type-checking related flags
7c21111 
It is a bug of the current PpxContext that -rectypes is not passed as
part of the context: any ppx extension that would like to be able to
load .cmi files in the same initial environment as the user code would
break because loading -rectypes-using .cmi is disallowed if
Clflags.recursive_types is not set.

(I found this issue while debugging a ppx_import user that compiles
with -rectypes -- ocaml-ppx/ppx_import#25 )

I tried to add all the other Clflags that seem related to
type-checking and might cause a program to not-type-check if they
are not correctly passed:

- recursive_types
- principal
- transparent_modules
- unboxed_types
- unsafe_string
@gasche
make the ppx-contexts tests more complete
08c48a5
@gasche gasche merged commit 6949695 into ocaml:trunk Jul 21, 2018
@vicuna vicuna mentioned this pull request Mar 14, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@let-def let-def let-def approved these changes

+1 more reviewer

@Drup Drup Drup approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@gasche @xclerc @let-def @Drup