racket / drracket

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

Fix error highlighting when background expansion is enabled #460

Closed sorawee closed 3 years ago

sorawee commented 3 years ago

Currently, under the following conditions:

  1. Errortrace is disabled
  2. Background expansion is enabled, and it has finished expanding the program
  3. The editor is unsaved (not named).

DrRacket will fail to highlight error. This is because the expanding place uses a dummy srcloc "unsaved editor" which does not match the port name of any editors, so the editors refuse to recognize the srcloc.

Because the port name of an editor is an uninterned symbol, passing it to the expanding place doesn't look like a good idea. Instead, we continue to use the dummy srcloc and adjust its source to be the port name later.

Fixes #455

sorawee commented 3 years ago

Looking at this again, I'm not convinced at all that this fix is at the right place. Maybe the fix should be in get-exn-source-locs? Or maybe the fix should be correcting all srcloc under exn, stack1, and stack2 even before passing them to get-exn-source-locs? I have no idea what's the right thing to do.

rfindler commented 3 years ago

I'm getting a bit lost on the code that's actually in the (unsaved) definitions window. Is it the example in these docs is the one that we're talking about?

sorawee commented 3 years ago

Yes.

rfindler commented 3 years ago

The problematic case seems to be when DrRacket sends the already-compiled program back to be evaluated, i.e. when compiled-bytes isn't #f.

rfindler commented 3 years ago

Given that this funny value that's meant to represent the source location of an unsaved definitions window can just be embedded into the compiled object, it seems like this has to be made more "official", perhaps via a specific prefab struct that gets documented somewhere and various places adjusted to work with that instead of the "hidden" unique value.

sorawee commented 3 years ago

Ideally, running with or without background expansion (that is, through expanding place or not) should produce the same result.

When an unsaved editor is run without background expansion, it uses an uninterned symbol unsaved-editor as the source location. Would the protocol that you propose affect this too?

IIUC, the reason it is uninterned is to guarantee uniqueness of port name of each editor. But prefab structure would kill this guarantee, right?

rfindler commented 3 years ago

Ideally, running with or without background expansion (that is, through expanding place or not) should produce the same result.

Yes.

When an unsaved editor is run without background expansion, it uses an uninterned symbol unsaved-editor as the source location. Would the protocol that you propose affect this too?

Yes.

IIUC, the reason it is uninterned is to guarantee uniqueness of port name of each editor. But prefab structure would kill this guarantee, right?

Yes.

There doesn't seem to be a way to "protect" the identity of the definitions window in the sources so we'd need to reveal more information (which is what I meant by my "coded" comment about documentation).

sorawee commented 3 years ago

I will close this PR then, since its approach is clearly wrong. I might revisit this bug later if it's not already been fixed.

rfindler commented 3 years ago

The more I'm thinking about this, the more I think that it is fine to be able to "forge" being in the definitions window as the source. After all, you can already "forge" arbitrary paths. So I'm thinking that declaraing that some specific prefab (with some integer or pair of integes in it) will just be interpreted as belonging to a specific definitions window is probably the way to go. There is some question as to what that means when you save one of these and restart drracket and manage to get an old one, but perhaps that's just life.

rfindler commented 3 years ago

Hm. I see that get-port-name is already part of a public API. So maybe what I have to do is just encode more structure into that symbol and stop it from being a unique symbol.