Qualify locally-bound names with source spans

[purefunctor]

Description of the change

Closes #4287. This adds a new desugaring step, desugarLocals, which turns references to local bindings be qualified by the source spans of the said bindings they're referring back to. Note that this had to be a separate step from desugarImports as it also needs to take into account binding groups. This also doesn't introduce new tests as an incorrect implementation breaks all other compiler tests quite heavily.

---

Checklist:

• Added a file to CHANGELOG.d for this PR (see CHANGELOG.d/README.md)
  [ ] Added myself to CONTRIBUTORS.md (if this is my first contribution)
• Linked any existing issues or proposals that this pull request should close
  [ ] Updated or added relevant documentation
• Added a test for the contribution (if applicable)

[JordanMartinez]

@purefunctor Just to clarify, is this a PR that has breaking changes that we may need to consider including in 0.15.0? Or can this wait until after 0.15.0 is released? I recall you making a comment a week ago about wanting to get another PR in with 0.15.0.

[purefunctor]

@JordanMartinez Yeah, this is breaking change for consumers of the compiler as a library. In particular, it changes the signature for the Qualified data structure such that it has more information.

I'm just curious, have there been any instances where minor releases included breaking changes for the public-facing API of the compiler library? I'm fine with getting this potentially merged in after 0.15.0, if it also means bundling this with #4247.

[JordanMartinez]

@purefunctor Gotcha. Thanks for clarifying.

Yeah, this is breaking change for consumers of the compiler as a library. In particular, it changes the signature for the Qualified data structure such that it has more information.

AFAIK, the only consumers at this point are:

• trypurescript
• pursuit
• potentially backends that use a fork of the purescript project to implement their backend?
• language-server tools (but these likely don't directly depend on this project)

I'm not sure what our policy is for breaking changes that affect consumers of the library but not necessarily users of the language.

[JordanMartinez]

I'm not sure what our policy is for breaking changes that affect consumers of the library but not necessarily users of the language.

Just wanted to say that changes are considered breaking if they affect "compiler-the-application" users, not "compiler-the-library" users. We need to document this more explicitly somewhere, but the short answer is this change isn't a breaking one.

[JordanMartinez]

This overall LGTM. The merge conflict and the source span comment needs to be resolved. Another question is whether we should try to merge another PR first to reduce merge conflicts.

[nwolverson]

If it's a breaking change to corefn that should be considered on its own merits - currently using purescript as a library for corefn, so the details don't matter. Have version dependency for other reasons anyway

…

On Fri, 20 May 2022, 14:38 JordanMartinez, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/Language/PureScript/CoreFn/ToJSON.hs <#4293 (comment)> : > + -- TODO: Should `sourceSpanXJSON` encode this information as well? + [ T.pack "spanName" .= toJSON (spanName ss) @nwolverson <https://github.com/nwolverson> would this be something that affects the purerl backend? — Reply to this email directly, view it on GitHub <#4293 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAVEPSY7A37TYIODRIOWBDDVK6IVVANCNFSM5TUDJEKA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[purefunctor]

I just synced back to master for this PR and fixed the compilation errors that it introduced. Unfortunately, the implementation has been left in a broken state and I've a lot of work to do for this to be merged.

[purefunctor]

Tests compile locally and aside from the comment about source span names, this should be ready for review.

[purefunctor]

This PR is ready for review 🎉

[JordanMartinez]

LGTM. I had a few questions and small nitpicks before approving this.

[rhendric]

Can't remember if I asked this before, but why a SourceSpan instead of a SourcePos? I'd think that the extra information contained by a SourceSpan is all redundant—we don't need a file name because it's necessarily a reference to an identifier in the same file, and we know the span represents an occurrence of a given identifier so the length of the identifier is enough to determine where the end position is from the start position. Given that we serialize the representation of qualified names a lot, I'd rather not bloat up that representation unnecessarily.

[purefunctor]

Can't remember if I asked this before, but why a SourceSpan instead of a SourcePos?

I can't remember exactly if it was intentional of me to use a SourceSpan or I just got confused with the types 😄.

Given that we serialize the representation of qualified names a lot, I'd rather not bloat up that representation unnecessarily.

But yes, this is a valid concern that I'd missed and I could refactor the PR today to accommodate for that 🙂

[rhendric]

AST changes and the changes downstream of that all look good. Assuming there's a good reason I didn't understand for making a distinct pass for the renaming logic, just a few minor quibbles there; if there isn't a good reason, I'd prefer to see this integrated into the existing pass, since said pass duplicates a lot of the same ideas (keeping track of bound variables and updating them) and wouldn't require a new generic AST traversal to be defined.

[JordanMartinez]

Just a thought, but should we distinguish ByNullSourcePos as its own data constructor?

Also, on looking at this again, there's not a test that verifies that a given local binding has the expected BySourcePos. Could one be added?

[purefunctor]

Just a thought, but should we distinguish ByNullSourcePos as its own data constructor?

@JordanMartinez We could, but it also means duplicating code that's already caught by BySourcePos, and changing most of them would take some work.

Also, on looking at this again, there's not a test that verifies that a given local binding has the expected BySourcePos. Could one be added?

Maybe we could check through the CoreFn? I also mentioned before that any incorrectness in the renamer is likely going to be caught during type checking because QualifiedBy is used as a key in the environment.

[JordanMartinez]

Just a thought, but should we distinguish ByNullSourcePos as its own data constructor?

@JordanMartinez We could, but it also means duplicating code that's already caught by BySourcePos, and changing most of them would take some work.

Also, on looking at this again, there's not a test that verifies that a given local binding has the expected BySourcePos. Could one be added?

Maybe we could check through the CoreFn? I also mentioned before that any incorrectness in the renamer is likely going to be caught during type checking because QualifiedBy is used as a key in the environment.

Makes sense. Thanks for explaining!