search

racket / drracket

DrRacket, IDE for Racket
http://www.racket-lang.org/
Other
454 stars 93 forks source link

Fix renaming in DrRacket #415

Closed sorawee closed 4 years ago

sorawee commented 4 years ago

Renaming in DrRacket is currently broken in two ways.

  1. Consider the program:

    lang racket

    (require racket/list)

    Right clicking require will offer users to rename the racket identifier which doesn't make much sense. Users right click require because they want to rename that particular identifier, not others.

  1. To continue from the above example, renaming racket to racket/base will cause all identifiers imported from racket to be renamed to racket/base. That is, it would result in:

    lang racket/base

    (require/base racket/list)

    This bug will become even more annoying after racket/racket#3391 is merged, since renaming any defined identifier will cause all-defined-out to be renamed as well.

This PR fixes both issues. That is, renaming at a position will pick an identifier from that position, and renaming an identifier will only cause identifiers that are textually equivalent to that identifier to be renamed.

rfindler commented 4 years ago

I am not sure about this -- I never thought that we should compare the characters in the original program! It seems like it might be that someone somewhere has a language that this would break if they, for example, equate variables with and without accents of them or ... who knows what else.

Still, for the majority of things (everything I actually know about) this seems like a great idea!

I'm not sure if it is necessary but we could add a backwards compatibility hedge in the form of a property that would say "rename even if the name is different" that could be plumbed through?

And we probably do need some tests (this time I'm just not missing them am I?).

sorawee commented 4 years ago

CC: @greghendershott

I am not sure about this -- I never thought that we should compare the characters in the original program! It seems like it might be that someone somewhere has a language that this would break if they, for example, equate variables with and without accents of them or ... who knows what else.

That's a good point. One instance I can see is when read-case-sensitive is #f (such as #lang r5rs). This PR would make it not possible to rename A in:

#lang r5rs
(define a 1)
A

There are two problems at hand: (1) how can language + macro indicate when it's safe (or unsafe) to rename an identifier? (2) how can Check Syntax provide these information to clients (e.g., DrRacket, Racket Mode) in a backward-compatible manner?

For (2), Racket Mode currently uses syncheck:add-arrow/name-dup/pxpy to decide whether or not a variable is rename-able. For example, Racket Mode deliberately restricts renaming to local variables (require-arrow in syncheck:add-arrow/name-dup/pxpy is #f). This suggests that the fix is to extend syncheck:add-arrow/name-dup/pxpy to syncheck:add-arrow/name-dup/pxpy/renamable. The additional argument renamable? would indicate if renaming either end should affect the other end. When require-arrow is not #f, renamable? will be #f.

Racket Mode would implement both syncheck:add-arrow/name-dup/pxpy and syncheck:add-arrow/name-dup/pxpy/renamable to support both old and new versions of DrRacket. At runtime, it can first check if syncheck-annotations<%> has a method syncheck:add-arrow/name-dup/pxpy/renamable, and if it does, syncheck:add-arrow/name-dup/pxpy would be disabled. When syncheck:add-arrow/name-dup/pxpy/renamable is not in the interface, syncheck:add-arrow/name-dup/pxpy/renamable would simply be unused.

For (1), since currently there's no restriction, it seems to me that to make it backward compatible, it should be an opt-out mechanism. That is, all local connections would make renamable? #t by default, but a syntax property inhibit-rename? setting to #t of identifiers in either disappeared-binding or disappeared-use would make renamable? #f.

And we probably do need some tests (this time I'm just not missing them am I?).

Yes, I will add tests.

sorawee commented 4 years ago

@rfindler: To prevent the PR from becoming massive, should I make a change to only include the uncontroversial part of the this PR (the first issue in the top post), and separate the rest (the second issue in the top post) as a separate PR?

rfindler commented 4 years ago

I'm not sure exactly what you have in mind with the split but I do agree with the general principle that getting uncontroversial improvements in their own PRs and merging them is Very Good.

sorawee commented 4 years ago

Well, I just want to make it easy for you to review.

Do you have any comment about my above plan of adding syncheck:add-arrow/name-dup/pxpy/renamable?

sorawee commented 4 years ago

OK, I removed the "compare the characters in the original program" from the PR and added tests. All tests passed, so it's ready for a review (and merge). I will submit a separate, draft PR that implements syncheck:add-arrow/name-dup/pxpy/renamable soon.

Thanks!