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

Support nested file imports in LSP #616

Merged
merged 6 commits into from
Feb 25, 2021

Conversation

psiemens
Copy link
Contributor

@psiemens psiemens commented Feb 22, 2021

Description

I noticed a small bug that occurs when using nested imports in the VS Code extension (e.g. import a file that imports another file).

Here's an example:

contracts/
  MyContractA.cdc
  MyContractB.cdc
transactions/
  myTransaction.cdc
// transactions/myTransaction.cdc

import MyContractA from "../contracts/MyContractA.cdc"
// contracts/MyContractA.cdc

import MyContractB from "./MyContractB.cdc"

In this case, when resolving the import of MyContractB, the mainPath value is still transactions/myTransaction.cdc, so the computed absolute file path (I call this path normalization) is transactions/MyContractB.cdc, which fails.

This PR updates the import logic to normalize paths before setting the location on the sub-checker for the imported program. This way, each value of checker.Location points to the absolute path for that file. Then, any further nested imports are normalized against checker.Location, allowing for arbitrary depth of imports.

Disclaimer: I have only tested this code with file imports and have not yet tested extensively enough to ensure I didn't introduce a regression with address-based imports. I'll try to find time for that today.


For contributor use:

  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you for fixing the local file resolution, I hadn't realized it wasn't handling relative paths properly 👍

Just one concern, and there seem to be build errors

languageserver/server/server.go Outdated Show resolved Hide resolved
runtime/sema/errors.go Outdated Show resolved Hide resolved
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! Thank you for fixing this 🙏

@psiemens psiemens added this to the Sprint 32 - Post Beta Mainnet - Part 14 [Feb 15 -Feb 26] milestone Feb 25, 2021
@psiemens psiemens force-pushed the languageserver-resolve-file-imports branch from 2a1e1e0 to 5f8226f Compare February 25, 2021 18:47
@psiemens psiemens merged commit f28ae63 into master Feb 25, 2021
@psiemens psiemens deleted the languageserver-resolve-file-imports branch February 25, 2021 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants