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

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

Merged
merged 2 commits into from Jul 21, 2018

Conversation

Projects
None yet
4 participants
@gasche
Copy link
Member

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

Copy link
Contributor

left a comment

I agree. The patch looks good.

@xclerc

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2018

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

@gasche

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2018

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

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 gasche:type-flags-in-ppx-context branch from cda0df0 to 08c48a5 Jul 20, 2018

@gasche

This comment has been minimized.

Copy link
Member Author

commented Jul 20, 2018

(I just rebased to add a Changes entry.)

gasche added some commits Jul 20, 2018

PpxContext: store type-checking related flags
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 gasche merged commit 6949695 into ocaml:trunk Jul 21, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@vicuna vicuna referenced this pull request Mar 14, 2019

Closed

Bug #1921 #8497

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.