Issue 110

[greghendershott]

As the comment for commit 003f87d explains, there are some caveats. It took me quite a long time to understand traversal.rkt well enough to come up with this proposed fix. I wouldn't be offended at all if someone took a glance, threw it away, and fixed it in some more-correct and more-elegant way that I can't see.

[rfindler]

I think there probably is an improvement we could make here but the (non-whitespace) change in this PR causes this program to not have any arrow:

#lang racket/base
(require (rename-in racket/list [first 1st]))
1st

[rfindler]

I've pushed the whitespace commit.

[greghendershott]

Thanks for pointing that out.

• Since you already merged the indentation commit to master, I rebased this onto master.

• The new, single commit in this PR:

  • Populates phase-to-requires exactly as it did before. Instead, it does the more-specific-syntax business only when using the results, in color-unused. (Unfortunately, that reverts to the old behavior of producing many redundant annotations for combine-in; reducing those would need to be done some other way.)

  • Adds a test for the desired behavior, as well as not regressing in the way my first attempt did.

Is this better?

[greghendershott]

p.s. It would be handy if tests ran automatically for pull requests on this repo, as happens for many repos including the main Racket repo. (It wouldn't necessarily need to do the whole thing Racket does, with multiple CI services for multiple platforms. Even a simple run of the tests using Travis or GitHub Actions, would be a handy "smoke test".)

[greghendershott]

@samth Oh, thank you! It looks like I could have submitted this kind of change, myself, if I'd known more about GitHub Actions. (With Travis CI, AFAIK a repo owner needs to enable that; I wrongly assumed it would be similar for GHA.)

[samth]

I merged the CI, so if you rebase on top of that, it will be included. However, syncheck-test.rkt has a persistent failure in CI so I didn't include that.

[samth]

Also, GitHub Actions are enabled on all repositories in the racket organization.

[rfindler]

@samth I don't see a persistent failure in syncheck-test.rkt on drdr. Is there somewhere else I should be looking? (tia)

@greghendershott it isn't clear to me that this is a correct change. (Of course, if we were making the exact opposite change (going from mod-stx to stx for source locations) I would say the same thing!) I'll need to think about it and try out stuff to try to see if I can come to a conclusion. Sorry for the delay.

[samth]

@rfindler the failure was only when I tried it in GitHub Actions; here's an example: https://github.com/samth/drracket/runs/537499701?check_suite_focus=true#step:6:22

Note also the issue with the autosave file, it was fixed with these lines: https://github.com/racket/drracket/blob/master/.github/workflows/ci.yml#L28-L29 but maybe DrR should handle that directory not existing/not being writable?

[greghendershott]

@samth:

I merged the CI, so if you rebase on top of that, it will be included.

Done.

[rfindler]

@samth can you try it again with the code I just pushed? I'm not sure it if will fix it, but hopefully we'll get at least a different error message.

[samth]

I just tried a new PR with that.

[samth]

Once I ironed out the directory permissions issues, it seemed to pass, so I merged syncheck-test in CI.

[greghendershott]

Sorry for the multiple force-pushes. I needed one to rebase this onto latest master. I needed a second because I realized I hadn't resolved the merge conflict correctly, and fixed that. (The third? Caffeine deficiency, idk.)

[rfindler]

Thanks, @greghendershott . I have started looking into this but didn't find enough time last week to make real progress. I didn't forget, tho!

[greghendershott]

No worries. Not urgent from my POV. I just wanted to keep the PR from getting stale, in case it happens to be something you think would be correct to merge.

[rfindler]

Okay, I've had time to look into this and I see why this is a good change. Thanks!

One variation: I think it should probably use stx instead of mod-stx in the case that mod-stx doesn't have a source-syntax field (so we prefer mod-stx in the case that it comes from somewhere that seems relevant).

A larger change, for down the road, would be to tell the traversal code what value of the source field is relevant (there seems to always be exactly one) and then we can be more aggressive/accurate in code like this. But in the meantime, it seems easier to argue that that version fo the change isn't going to lose interesting information.

If that sounds okay to you, @greghendershott , I'll go ahead and make that change (and I'll also use define-get-arrows to simplify the test cases).

[greghendershott]

Yes, I agree that seems reasonable: Prefer the module syntax if it has non-#f source, position, and span -- else use the full syntax.

[greghendershott]

Just out of curiosity, N/A for the PR:

A larger change, for down the road, would be to tell the traversal code what value of the source field is relevant (there seems to always be exactly one) and then we can be more aggressive/accurate in code like this.

Are you thinking about something where things outside of drracket/check-syntax can do this, like via a syntax property? (Because that might turn out to be the only way to fix some "unused red herrings", e.g. some issues with all-defined-out, racket/contract or racket/class?)

[rfindler]

Just out of curiosity, N/A for the PR:

A larger change, for down the road, would be to tell the traversal code what value of the source field is relevant (there seems to always be exactly one) and then we can be more aggressive/accurate in code like this.

Are you thinking about something where things outside of drracket/check-syntax can do this, like via a syntax property? (Because that might turn out to be the only way to fix some "unused red herrings", e.g. some issues with all-defined-out, racket/contract or racket/class?)

I'm saying that somewhere the API for using drracket/check-syntax would widen to allow the uses of it to supply the name of the file that is "of interest" to streamline the internals so that they could just ignore syntax objects that don't have a particular syntax-source field. I am sensitive to the concern that you care about one use of the API working across many versions of Racket so we would have to think hard about how to extend the API (like adding an optional argument that has an especially "smart" default perhaps?). I would normally add an optional argument that would preserve the current behavior but perhaps that's not the right call this time.

I didn't see this as a way to fix a specific bug per se, but it would be great if it did! (There is also a bug somewhere that types show up in the wrong files.)

Anyway, this isn't near the top my list! :)

[greghendershott]

Oh I misunderstood your concern was that the bit of syntax for the module path might lack a position and span, for some reason. Instead you meant, what if lacks any source at all.

Although I can't imagine that scenario, it sounds like you can!

By the way, I'm not sure that syntax-source being non-#f necessarily means that it is "relevant"? For that, would you want to check that the syntax-source of the module path syntax equals that of the entire require subform syntax -- they "both come from the same source"? i.e. Instead of (define mod-stx (if (syntax-source raw-mod-stx) raw-mod-stx stx)) something like (define mod-stx (if (equal? (syntax-source raw-mod-stx) (syntax-source stx) raw-mod-stx stx))? Again, I don't know the scenario you have in mind, so not sure if this matters.

[greghendershott]

p.s. Speaking of "irrelevant sources", that reminds me of #230 (although that's about the step debugger not check-syntax).

[rfindler]

Oh I misunderstood your concern was that the bit of syntax for the module path might lack a position and span, for some reason. Instead you meant, what if lacks any source at all.

They can certainly vary independently, but in practice I think that it tends to have a useful source or nothing.

Although I can't imagine that scenario, it sounds like you can!

It used to be more common when source from syntax objects weren't saved in zo files, for sure. But you can certainly just make one.

By the way, I'm not sure that syntax-source being non-#f necessarily means that it is "relevant"?

Right, agreed. It isn't necessary.

For that, would you want to check that the syntax-source of the module path syntax equals that of the entire require subform syntax -- they "both come from the same source"? i.e. Instead of (define mod-stx (if (syntax-source raw-mod-stx) raw-mod-stx stx)) something like (define mod-stx (if (equal? (syntax-source raw-mod-stx) (syntax-source stx) raw-mod-stx stx))? Again, I don't know the scenario you have in mind, so not sure if this matters.

What I think a good predicate for relevance is "which source corresponds to the file in the emacs buffer that I first sent to the drracket/check-syntax library?" That's not what's happening here, for sure and it would be good to get there eventually.