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.

Sign up for GitHub

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

Jump to bottom

construct: Do not produce redundant prefixes #1618

Merged
merged 3 commits into from
Jun 22, 2023
Merged

construct: Do not produce redundant prefixes #1618

merged 3 commits into from
Jun 22, 2023

Conversation

bcc32
Copy link
Contributor

@bcc32 bcc32 commented May 30, 2023

As suggested in #1552, there shouldn't be any need to generate module prefixes for constructors or record fields when using construct, since the hole's type can be used with type-directed disambiguation.

Closes #1552.

@voodoos voodoos self-requested a review June 12, 2023 17:46
@voodoos
Copy link
Collaborator

voodoos commented Jun 13, 2023 •
edited
Loading

Right, it sounds like a reasonable assumption and I cannot think of a counter example so far.
However we probably should keep the prefix when the warning 42, disambiguated-name is active.

@bcc32
Copy link
Contributor Author

bcc32 commented Jun 13, 2023

Seems reasonable to me. Is there a way to read this information inside merlin? I tried just calling Warnings.is_active (Disambiguated_name "") but this always returns false for me, even in tests where I'm passing -w +42 or similar.

@voodoos
Copy link
Collaborator

voodoos commented Jun 16, 2023

I don't know either, I will have a look !

@voodoos
Copy link
Collaborator

voodoos commented Jun 16, 2023

The Mconfig.t record contains a field ocaml.warnings of type Warnings.t. I guess you need to wrap your call to is_active with Warning.with_state.

@bcc32
Copy link
Contributor Author

bcc32 commented Jun 16, 2023

OK, I took some guesses as to how best to thread the config value through to the construct command. Hopefully my changes make sense but please feel free to ask for any modifications.

voodoos
voodoos approved these changes Jun 19, 2023
View reviewed changes
Copy link
Collaborator

@voodoos voodoos left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, I just have a last nit-pick :-)

src/analysis/construct.ml Outdated Show resolved Hide resolved
src/analysis/construct.ml Outdated Show resolved Hide resolved
@bcc32
construct: Do not produce redundant prefixes
05e1b68 
When construct is called, there is always enough type information for
type-directed disambiguation to find the appropriate constructor.

This changes stops generating prefixes for constructors and record
field names.

Closes ocaml#1552.
@bcc32
Only omit prefix conditionally
0b48ba0
@bcc32
Refactor omit_if_possible argument in Util.prefix
fe1194e
@voodoos voodoos merged commit 106156f into ocaml:master Jun 22, 2023
@voodoos
Copy link
Collaborator

voodoos commented Jun 22, 2023

Thank you !

@bcc32 bcc32 deleted the omit-module-prefixes branch June 22, 2023 18:23
voodoos added a commit to voodoos/merlin that referenced this pull request Aug 24, 2023
@voodoos
[B] construct: Do not produce redundant prefixes (ocaml#1618)
cf52276 
from bcc32/omit-module-prefixes
voodoos added a commit to voodoos/opam-repository that referenced this pull request Aug 24, 2023
@voodoos
[new release] merlin, merlin-lib and dot-merlin-reader (4.10-414)
2bc5e35 
CHANGES:

Thu Aug 24 17:17:42 CEST 2023

  + merlin binary
    - Constrain socket path buffer size to avoid build warnings (ocaml/merlin#1631)
    - Handle concurrent server start (ocaml/merlin#1622)
    - Omit module prefixes for constructors and record fields in the
      `construct` command (ocaml/merlin#1618).  Prefixes are still produced when
      warning 42 (disambiguated name) is active.
    - Correctly invalidate PPX cache when pipeline ran partially (ocaml/merlin#1650,
      fixes ocaml/merlin#1647)
    - Prevent `short-path` from looping in some cases related to recursive type
      definitions (ocaml/merlin#1645)
    - Support parsing negative numbers in sexps (ocaml/merlin#1655)
    - Fix construct not working with inline records (ocaml/merlin#1658)
    - Improve behavior of `type-enclosing` on let/and operators (ocaml/merlin#1653)
    - Fix occurrences of extension constructors (ocaml/merlin#1662)
    - Improve node selection when ghosts are present (ocaml/merlin#1664, fixes ocaml/merlin#1660)
  + editor modes
    - emacs: call merlin-client-logger with "interrupted" if the
      merlin binary itself is interrupted, not just the parsing of the
      result (ocaml/merlin#1626).
    - emacs: merlin-construct, with a prefix argument, now includes
      local values in the completion options.  Alternatively, this
      behavior can be enabled permanently by customizing
      `merlin-construct-with-local-values` (ocaml/merlin#1644)
    - emacs: add support for opam-switch-mode (ocaml/merlin#1654, fixes ocaml/merlin#1591).
      See <https://github.com/ProofGeneral/opam-switch-mode>
voodoos added a commit to voodoos/opam-repository that referenced this pull request Aug 24, 2023
@voodoos
[new release] merlin, merlin-lib and dot-merlin-reader (4.10-500)
e56b3a0 
CHANGES:

Thu Aug 24 17:17:42 CEST 2023

  + merlin binary
    - Constrain socket path buffer size to avoid build warnings (ocaml/merlin#1631)
    - Handle concurrent server start (ocaml/merlin#1622)
    - Omit module prefixes for constructors and record fields in the
      `construct` command (ocaml/merlin#1618).  Prefixes are still produced when
      warning 42 (disambiguated name) is active.
    - Correctly invalidate PPX cache when pipeline ran partially (ocaml/merlin#1650,
      fixes ocaml/merlin#1647)
    - Prevent `short-path` from looping in some cases related to recursive type
      definitions (ocaml/merlin#1645)
    - Support parsing negative numbers in sexps (ocaml/merlin#1655)
    - Fix construct not working with inline records (ocaml/merlin#1658)
    - Improve behavior of `type-enclosing` on let/and operators (ocaml/merlin#1653)
    - Fix occurrences of extension constructors (ocaml/merlin#1662)
    - Improve node selection when ghosts are present (ocaml/merlin#1664, fixes ocaml/merlin#1660)
  + editor modes
    - emacs: call merlin-client-logger with "interrupted" if the
      merlin binary itself is interrupted, not just the parsing of the
      result (ocaml/merlin#1626).
    - emacs: merlin-construct, with a prefix argument, now includes
      local values in the completion options.  Alternatively, this
      behavior can be enabled permanently by customizing
      `merlin-construct-with-local-values` (ocaml/merlin#1644)
    - emacs: add support for opam-switch-mode (ocaml/merlin#1654, fixes ocaml/merlin#1591).
      See <https://github.com/ProofGeneral/opam-switch-mode>
voodoos added a commit to voodoos/opam-repository that referenced this pull request Aug 24, 2023
@voodoos
[new release] merlin, merlin-lib (4.10-414)
f445238 
CHANGES:

Thu Aug 24 17:17:42 CEST 2023

  + merlin binary
    - Constrain socket path buffer size to avoid build warnings (ocaml/merlin#1631)
    - Handle concurrent server start (ocaml/merlin#1622)
    - Omit module prefixes for constructors and record fields in the
      `construct` command (ocaml/merlin#1618).  Prefixes are still produced when
      warning 42 (disambiguated name) is active.
    - Correctly invalidate PPX cache when pipeline ran partially (ocaml/merlin#1650,
      fixes ocaml/merlin#1647)
    - Prevent `short-path` from looping in some cases related to recursive type
      definitions (ocaml/merlin#1645)
    - Support parsing negative numbers in sexps (ocaml/merlin#1655)
    - Fix construct not working with inline records (ocaml/merlin#1658)
    - Improve behavior of `type-enclosing` on let/and operators (ocaml/merlin#1653)
    - Fix occurrences of extension constructors (ocaml/merlin#1662)
    - Improve node selection when ghosts are present (ocaml/merlin#1664, fixes ocaml/merlin#1660)
  + editor modes
    - emacs: call merlin-client-logger with "interrupted" if the
      merlin binary itself is interrupted, not just the parsing of the
      result (ocaml/merlin#1626).
    - emacs: merlin-construct, with a prefix argument, now includes
      local values in the completion options.  Alternatively, this
      behavior can be enabled permanently by customizing
      `merlin-construct-with-local-values` (ocaml/merlin#1644)
    - emacs: add support for opam-switch-mode (ocaml/merlin#1654, fixes ocaml/merlin#1591).
      See <https://github.com/ProofGeneral/opam-switch-mode>
voodoos added a commit to voodoos/opam-repository that referenced this pull request Aug 24, 2023
@voodoos
[new release] merlin, merlin-lib (4.10-500)
38b91b3 
CHANGES:

Thu Aug 24 17:17:42 CEST 2023

  + merlin binary
    - Constrain socket path buffer size to avoid build warnings (ocaml/merlin#1631)
    - Handle concurrent server start (ocaml/merlin#1622)
    - Omit module prefixes for constructors and record fields in the
      `construct` command (ocaml/merlin#1618).  Prefixes are still produced when
      warning 42 (disambiguated name) is active.
    - Correctly invalidate PPX cache when pipeline ran partially (ocaml/merlin#1650,
      fixes ocaml/merlin#1647)
    - Prevent `short-path` from looping in some cases related to recursive type
      definitions (ocaml/merlin#1645)
    - Support parsing negative numbers in sexps (ocaml/merlin#1655)
    - Fix construct not working with inline records (ocaml/merlin#1658)
    - Improve behavior of `type-enclosing` on let/and operators (ocaml/merlin#1653)
    - Fix occurrences of extension constructors (ocaml/merlin#1662)
    - Improve node selection when ghosts are present (ocaml/merlin#1664, fixes ocaml/merlin#1660)
  + editor modes
    - emacs: call merlin-client-logger with "interrupted" if the
      merlin binary itself is interrupted, not just the parsing of the
      result (ocaml/merlin#1626).
    - emacs: merlin-construct, with a prefix argument, now includes
      local values in the completion options.  Alternatively, this
      behavior can be enabled permanently by customizing
      `merlin-construct-with-local-values` (ocaml/merlin#1644)
    - emacs: add support for opam-switch-mode (ocaml/merlin#1654, fixes ocaml/merlin#1591).
      See <https://github.com/ProofGeneral/opam-switch-mode>
@voodoos voodoos mentioned this pull request Aug 24, 2023
Merged
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
@voodoos @nberth
[new release] merlin, merlin-lib (4.10-414)
dccf2b8 
CHANGES:

Thu Aug 24 17:17:42 CEST 2023

  + merlin binary
    - Constrain socket path buffer size to avoid build warnings (ocaml/merlin#1631)
    - Handle concurrent server start (ocaml/merlin#1622)
    - Omit module prefixes for constructors and record fields in the
      `construct` command (ocaml/merlin#1618).  Prefixes are still produced when
      warning 42 (disambiguated name) is active.
    - Correctly invalidate PPX cache when pipeline ran partially (ocaml/merlin#1650,
      fixes ocaml/merlin#1647)
    - Prevent `short-path` from looping in some cases related to recursive type
      definitions (ocaml/merlin#1645)
    - Support parsing negative numbers in sexps (ocaml/merlin#1655)
    - Fix construct not working with inline records (ocaml/merlin#1658)
    - Improve behavior of `type-enclosing` on let/and operators (ocaml/merlin#1653)
    - Fix occurrences of extension constructors (ocaml/merlin#1662)
    - Improve node selection when ghosts are present (ocaml/merlin#1664, fixes ocaml/merlin#1660)
  + editor modes
    - emacs: call merlin-client-logger with "interrupted" if the
      merlin binary itself is interrupted, not just the parsing of the
      result (ocaml/merlin#1626).
    - emacs: merlin-construct, with a prefix argument, now includes
      local values in the completion options.  Alternatively, this
      behavior can be enabled permanently by customizing
      `merlin-construct-with-local-values` (ocaml/merlin#1644)
    - emacs: add support for opam-switch-mode (ocaml/merlin#1654, fixes ocaml/merlin#1591).
      See <https://github.com/ProofGeneral/opam-switch-mode>
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
@voodoos @nberth
[new release] merlin, merlin-lib (4.10-500)
bc02e79 
CHANGES:

Thu Aug 24 17:17:42 CEST 2023

  + merlin binary
    - Constrain socket path buffer size to avoid build warnings (ocaml/merlin#1631)
    - Handle concurrent server start (ocaml/merlin#1622)
    - Omit module prefixes for constructors and record fields in the
      `construct` command (ocaml/merlin#1618).  Prefixes are still produced when
      warning 42 (disambiguated name) is active.
    - Correctly invalidate PPX cache when pipeline ran partially (ocaml/merlin#1650,
      fixes ocaml/merlin#1647)
    - Prevent `short-path` from looping in some cases related to recursive type
      definitions (ocaml/merlin#1645)
    - Support parsing negative numbers in sexps (ocaml/merlin#1655)
    - Fix construct not working with inline records (ocaml/merlin#1658)
    - Improve behavior of `type-enclosing` on let/and operators (ocaml/merlin#1653)
    - Fix occurrences of extension constructors (ocaml/merlin#1662)
    - Improve node selection when ghosts are present (ocaml/merlin#1664, fixes ocaml/merlin#1660)
  + editor modes
    - emacs: call merlin-client-logger with "interrupted" if the
      merlin binary itself is interrupted, not just the parsing of the
      result (ocaml/merlin#1626).
    - emacs: merlin-construct, with a prefix argument, now includes
      local values in the completion options.  Alternatively, this
      behavior can be enabled permanently by customizing
      `merlin-construct-with-local-values` (ocaml/merlin#1644)
    - emacs: add support for opam-switch-mode (ocaml/merlin#1654, fixes ocaml/merlin#1591).
      See <https://github.com/ProofGeneral/opam-switch-mode>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@voodoos voodoos voodoos approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Backport to 501
Status: 4.10-500
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

merlin-construct should shorten module paths
2 participants
@bcc32 @voodoos