Skip to content

Disable the local keyword in typing#540

Merged
mshinwell merged 1 commit intooxcaml:mainfrom
stedolan:disable-local-properly
Feb 21, 2022
Merged

Disable the local keyword in typing#540
mshinwell merged 1 commit intooxcaml:mainfrom
stedolan:disable-local-properly

Conversation

@stedolan
Copy link
Copy Markdown
Contributor

@stedolan stedolan commented Feb 21, 2022

If the local_ keyword somehow sneaks into the parsetree even with the extension disabled (e.g. because the parsing occured in a ppx), then the typechecker should nonetheless reject uses of it.

Tested manually with a temporarily hacked lexer that lets the keyword through always, example output:

# let f () = local_ ref 42;;
Error: The local extension is disabled
       To enable it, pass the '-extension local' flag

gretay-js
gretay-js reviewed Feb 21, 2022
View reviewed changes
@mshinwell mshinwell added the 4.12.0-7 Bug fix patches for 4.12.0-7 release label Feb 21, 2022
gretay-js
gretay-js approved these changes Feb 21, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@gretay-js gretay-js left a comment

Choose a reason for hiding this comment

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

I don't know anything about typing, but as far as this change goes, all it does is raise in a few places if Clflags.Extension.(is_enabled local) is false and a local attribute or extension is encountered, so it looks fine to me.

@xclerc
Copy link
Copy Markdown
Contributor

xclerc commented Feb 21, 2022

I am certainly out of my depth here, but there are occurrences
of "extension.local" in "ocaml/typing/typecore.ml" (lines 2848,
2977, and 5778) where we do not check whether the extension
is enabled. I assume this will be caught by surrounding code, but
I am having a hard time proving it to myself.

It is also not clear to me whether something should be done
about "extension.escape".

@stedolan
Copy link
Copy Markdown
Contributor Author

stedolan commented Feb 21, 2022

I am certainly out of my depth here, but there are occurrences of "extension.local" in "ocaml/typing/typecore.ml" (lines 2848, 2977, and 5778) where we do not check whether the extension is enabled. I assume this will be caught by surrounding code, but I am having a hard time proving it to myself.

It will. These three are in ancillary code that checks side-conditions (whether a function is annotated with local_ return, what the approximate type of a recursive value is, and whether an expression is a lambda, respectively). The code will always be typechecked by the main typechecking function after this, which has the check.

It is also not clear to me whether something should be done about "extension.escape".

Nothing! It's not a user-visible feature, it exists only temporarily during typechecking as a workaround for tricky compilation behaviour of optional arguments.

@xclerc
Copy link
Copy Markdown
Contributor

xclerc commented Feb 21, 2022

Sorry, unrelated to this diff but maybe we can sneak it in.
I think in "parsing/lexer.mll", is_keyword should have a
condition to accept/reject local_. It may also be possible
to recycle Extension_not_enabled rather than add a new
constructor, but that is certainly not blocking.

@stedolan stedolan force-pushed the disable-local-properly branch from cef4d42 to 61be403 Compare February 21, 2022 15:45
@stedolan stedolan requested a review from mshinwell as a code owner February 21, 2022 15:45
@mshinwell mshinwell merged commit fc69a96 into oxcaml:main Feb 21, 2022
mshinwell pushed a commit that referenced this pull request Feb 21, 2022
@stedolan @mshinwell
Disable the local keyword in typing (#540)
63dacb4
stedolan added a commit to stedolan/flambda-backend that referenced this pull request Mar 7, 2022
@stedolan
Squashed 'ocaml/' changes from 173842c..64235a3
c6bb9c5 
64235a3 flambda-backend: Change Float.nan from sNaN to qNaN (oxcaml#466)
14a8e27 flambda-backend: Track GC work for all managed bigarray allocations (upstream 11022) (oxcaml#569)
c3cda96 flambda-backend: Add two new methods to targetint for dwarf (oxcaml#560)
e6f1fed flambda-backend: Handle arithmetic overflow in select_addr (oxcaml#570)
dab7209 flambda-backend: Add Target_system to ocaml/utils (oxcaml#542)
82d5044 flambda-backend: Enhance numbers.ml with more primitive types (oxcaml#544)
216be99 flambda-backend: Fix flambda_o3 and flambda_oclassic attributes (oxcaml#536)
4b56e07 flambda-backend: Test naked pointer root handling (oxcaml#550)
40d69ce flambda-backend: Stop local function optimisation from moving code into function bodies; opaque_identity fixes for class compilation (oxcaml#537)
f08ae58 flambda-backend: Implemented inlining history and use it inside inlining reports (oxcaml#365)
ac496bf flambda-backend: Disable the local keyword in typing (oxcaml#540)
7d46712 flambda-backend: Bugfix for Typedtree generation of arrow types (oxcaml#539)
61a7b47 flambda-backend: Insert missing page table check in roots_nat.c (oxcaml#541)
323bd36 flambda-backend: Compiler error when -disable-all-extensions and -extension are used (oxcaml#534)
d8956b0 flambda-backend: Persistent environment and reproducibility (oxcaml#533)
4a0c89f flambda-backend: Revert "Revert bswap PRs (480 and 482)" (oxcaml#506)
7803705 flambda-backend: Cause a C warning when CAMLreturn is missing in C stubs. (oxcaml#376)
6199db5 flambda-backend: Improve unboxing during cmm for Flambda (oxcaml#295)
96b9e1b flambda-backend: Print diagnostics at runtime for Invalid (oxcaml#530)
42ab88e flambda-backend: Disable bytecode compilers in ocamltest (oxcaml#504)
58c72d5 flambda-backend: Backport ocaml/ocaml#10595 from upstream/trunk (oxcaml#471)
1010539 flambda-backend: Use C++ name mangling convention (oxcaml#483)
81881bb flambda-backend: Local allocation test no longer relies on lifting (oxcaml#525)
f5c4719 flambda-backend: Fix an assertion in Closure that breaks probes (oxcaml#505)
c2cf2b2 flambda-backend: Add some missing command line arguments to ocamlnat (oxcaml#499)

git-subtree-dir: ocaml
git-subtree-split: 64235a3
stedolan added a commit that referenced this pull request Mar 7, 2022
@stedolan
Squashed 'ocaml/' changes from 173842c..64235a3
b54d3a3 
64235a3 flambda-backend: Change Float.nan from sNaN to qNaN (#466)
14a8e27 flambda-backend: Track GC work for all managed bigarray allocations (upstream 11022) (#569)
c3cda96 flambda-backend: Add two new methods to targetint for dwarf (#560)
e6f1fed flambda-backend: Handle arithmetic overflow in select_addr (#570)
dab7209 flambda-backend: Add Target_system to ocaml/utils (#542)
82d5044 flambda-backend: Enhance numbers.ml with more primitive types (#544)
216be99 flambda-backend: Fix flambda_o3 and flambda_oclassic attributes (#536)
4b56e07 flambda-backend: Test naked pointer root handling (#550)
40d69ce flambda-backend: Stop local function optimisation from moving code into function bodies; opaque_identity fixes for class compilation (#537)
f08ae58 flambda-backend: Implemented inlining history and use it inside inlining reports (#365)
ac496bf flambda-backend: Disable the local keyword in typing (#540)
7d46712 flambda-backend: Bugfix for Typedtree generation of arrow types (#539)
61a7b47 flambda-backend: Insert missing page table check in roots_nat.c (#541)
323bd36 flambda-backend: Compiler error when -disable-all-extensions and -extension are used (#534)
d8956b0 flambda-backend: Persistent environment and reproducibility (#533)
4a0c89f flambda-backend: Revert "Revert bswap PRs (480 and 482)" (#506)
7803705 flambda-backend: Cause a C warning when CAMLreturn is missing in C stubs. (#376)
6199db5 flambda-backend: Improve unboxing during cmm for Flambda (#295)
96b9e1b flambda-backend: Print diagnostics at runtime for Invalid (#530)
42ab88e flambda-backend: Disable bytecode compilers in ocamltest (#504)
58c72d5 flambda-backend: Backport ocaml/ocaml#10595 from upstream/trunk (#471)
1010539 flambda-backend: Use C++ name mangling convention (#483)
81881bb flambda-backend: Local allocation test no longer relies on lifting (#525)
f5c4719 flambda-backend: Fix an assertion in Closure that breaks probes (#505)
c2cf2b2 flambda-backend: Add some missing command line arguments to ocamlnat (#499)

git-subtree-dir: ocaml
git-subtree-split: 64235a3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mshinwell mshinwell mshinwell approved these changes

@xclerc xclerc xclerc approved these changes

@gretay-js gretay-js gretay-js approved these changes

@lpw25 lpw25 Awaiting requested review from lpw25

@antalsz antalsz Awaiting requested review from antalsz

Assignees

No one assigned

Labels

4.12.0-7 Bug fix patches for 4.12.0-7 release typing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@stedolan @xclerc @mshinwell @gretay-js