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

"Remove Unused Requires" refactoring doesn't handle multi-in forms correctly #110

Closed
dfeltey opened this issue Apr 27, 2017 · 4 comments
Closed
Assignees

Comments

@dfeltey
Copy link
Collaborator

dfeltey commented Apr 27, 2017

Running the "Remove Unused Requires" refactoring on this program:

#lang racket/base

(require racket/require)
(require (multi-in racket (list dict)))
first

produces:

#lang racket/base

(require racket/require)
(require)
first

Rather than just removing the dict part of the multi-in form.

@dfeltey dfeltey self-assigned this Apr 27, 2017
@greghendershott
Copy link
Contributor

greghendershott commented Feb 15, 2020

Related: Dr Racket will color the entire (multi-in racket (list dict)) form red as "unused" -- even though list is used. In an ideal universe only the dict portion would correctly be marked red as unused.

I actually first noticed this yesterday while consuming syncheck:add-unused-require items from drracket/check-syntax for Racket Mode, then went to DrR to cross-check its behavior.


I wonder if it would be pragmatic and reasonable to change multi-in to expand to one (#%require) form per module? That's what (require racket/list racket/dict) would expand to, I think?

One the one hand drracket/check-syntax should ideally handle "everything". On the other hand if multi-in were from a third-party package, and its author learned there is a de facto, soft, unwritten protocol like this for tooling? They would probably adapt multi-in to use it. At least as a mitigation until/unless the tooling improves and/or the protocol becomes formal.

@greghendershott
Copy link
Contributor

It looks like multi-in currently expands to combine-in.

Using combine-in directly in the original source, also exhibits this bug.

(combine-in racket/list racket/dict) results in one big $%require form like this:

(#%require
 (just-meta 0 (rename racket/list filter-not filter-not))
 (just-meta 0 (rename racket/list list-prefix? list-prefix?))
 ...
 (just-meta 0 (rename racket/dict dict-set dict-set))
 (just-meta 0 (rename racket/dict dict-value-contract dict-value-contract))
 ... )

My guess is that any unused module path, like racket/dict in this case, will cause drracket/check-syntax to say that the entire #%require is unused.

I bet it would not happen with combine-in expanding instead to:

(#%require
 (just-meta 0 (rename racket/list filter-not filter-not))
 (just-meta 0 (rename racket/list list-prefix? list-prefix?))
 ...)

(#%require
 (just-meta 0 (rename racket/dict dict-set dict-set))
 (just-meta 0 (rename racket/dict dict-value-contract dict-value-contract))
 ... )

@greghendershott
Copy link
Contributor

Or maybe combine-in should be using syntax/loc on the pieces?

greghendershott added a commit to greghendershott/drracket that referenced this issue Mar 25, 2020
Also, avoid generating very many duplicate add-open-require and
unused-require annotations. This can happen when forms like combine-in
expand to rename sub-forms for every imported item. This should make
check-syntax quicker when used both by DrRacket and by things like
Racket Mode.

Caveats (i.e. please do review before merging):

- I'm not 100% sure I understand the usage of `phase-to-requires` so I
may have broken something else.

- I moved the generation of add-open-require annotations inside the
original-enough? test. It seems natural to handle dupe avoidance, for
those, at the same time we're handling it, for phase-to-requires. I'm
not sure how else to avoid those dupes, without introducing yet
another data structure.

So please do review. Even if my attempt to fix this, is poor,
hopefully I helped by narrowing down the cause (or at least proximate
cause) of the problem, and someone else could "fix it better".
greghendershott added a commit to greghendershott/drracket that referenced this issue Mar 25, 2020
Also, avoid generating very many duplicate add-open-require and
unused-require annotations. This can happen when forms like combine-in
expand to rename sub-forms for every imported item. This should make
check-syntax quicker when used both by DrRacket and by things like
Racket Mode.

Caveats (i.e. please do review before merging):

- I'm not 100% sure I understand the usage of `phase-to-requires` so I
may have broken something else.

- I moved the generation of add-open-require annotations inside the
original-enough? test. It seems natural to handle dupe avoidance, for
those, at the same time we're handling it, for phase-to-requires. I'm
not sure how else to avoid those dupes, without introducing yet
another data structure.

So please do review. Even if my attempt to fix this, is poor,
hopefully I helped by narrowing down the cause (or at least proximate
cause) of the problem, and someone else could "fix it better".
@greghendershott
Copy link
Contributor

I opened PR #362 with an effort to fix this within drracket/check-syntax.

greghendershott added a commit to greghendershott/drracket that referenced this issue Mar 26, 2020
greghendershott added a commit to greghendershott/drracket that referenced this issue Mar 26, 2020
greghendershott added a commit to greghendershott/drracket that referenced this issue Mar 26, 2020
greghendershott added a commit to greghendershott/drracket that referenced this issue Apr 6, 2020
greghendershott added a commit to greghendershott/drracket that referenced this issue Apr 6, 2020
greghendershott added a commit to greghendershott/drracket that referenced this issue Apr 6, 2020
rfindler added a commit that referenced this issue Apr 9, 2020
rfindler added a commit that referenced this issue Jan 31, 2023
…r need the special case that solved issue #110 anymore
rfindler added a commit that referenced this issue Feb 1, 2023
check that the symbolic name of a binder and its reference are the
same

Also, it appears that something changed along the way such that we no
longer need the special case that solved issue #110 anymore
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants