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

Fix language service panic when file isn't listed in the files field of qsharp.json #2109

Merged
merged 2 commits into from
Jan 24, 2025

Conversation

minestarks
Copy link
Member

@minestarks minestarks commented Jan 18, 2025

Fixes #2090

Notably, this also changes how we treat the files field in qsharp.json for local projects their local dependencies.

Previously, if the qsharp.json explicitly listed any files , that list would take priority and we would skip crawling the directory.

With this change: we always crawl the src/ directory and include all found *.qs files in the project. But we raise an explicit error to the user if any of those files are missing from the files list.

@minestarks minestarks force-pushed the minestarks/doc-not-in-files branch from a3d9843 to b24e63b Compare January 22, 2025 19:03
@minestarks minestarks changed the title fix language service panic when file is under the project folder but … Fix language service panic when file isn't listed in the files field of qsharp.json Jan 22, 2025
@minestarks minestarks marked this pull request as ready for review January 22, 2025 19:17
Copy link
Contributor

@sezna sezna 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, just left a few comments!

@minestarks minestarks enabled auto-merge January 24, 2025 22:06
@minestarks minestarks added this pull request to the merge queue Jan 24, 2025
Merged via the queue into main with commit e4ef02e Jan 24, 2025
18 checks passed
@minestarks minestarks deleted the minestarks/doc-not-in-files branch January 24, 2025 22:33
@fedimser
Copy link

fedimser commented Feb 2, 2025

Hello @minestarks!
Release of qsharp==1.13 broke build of our project without any changes on my side, and I beleive it's related to this PR.

On init(), we started getting this error for all source files, even though they all are explicitly listed in qsharp.json:

E       Exception: Qsc.Project.DocumentNotInProject
E       
E         x File src/EstimateUtils.qs is not listed in the `files` field of the
E         | manifest
E         help: To avoid unexpected behavior, add this file to the `files` field in
E               the `qsharp.json` manifest
E       

But removing all files from qsharp.json makes tests pass!

Does this mean we now are required to not list any .qs files if under src/ in the manifest? Or maybe there is a bug?

To reproduce this, you can clone this repo and run pytest . from the root.

@swernli
Copy link
Collaborator

swernli commented Feb 2, 2025

@fedimser Thanks for pointing us to the repo! I was able to try it in GitHub.dev (the extension works in the browser, even from the phone), and it looks like the project runs fine:
image

I wonder if it could be an issue with case sensitivity in the file system... are there any files or folders on your disk that are part of the project where the capitalization differs?

@fedimser
Copy link

fedimser commented Feb 2, 2025

The issue happens when you run pytest . , after pip install qsharp==1.13. Specifically, this happens in qsharp.init(). This happens both on my machine (Ubuntu) and on Github Actions, so I don't think it has to do with file names.

I tried to repro it on github.dev, but that would require installing qsharp=1.13, and the latest version available in that environment is 1.11.2.dev0.

@swernli
Copy link
Collaborator

swernli commented Feb 2, 2025

Ah, I will try to repro in a Python environment when I get the chance.

For the available versions of the extension, depending on how your VS Code Web instance is configured it might not automatically update to the latest. If you use the "Install specific version" command then it will bring up the full list of versions for you to choose from:
image

idavis pushed a commit that referenced this pull request Feb 2, 2025
…d of `qsharp.json` (#2109)

Fixes #2090

Notably, this also changes how we treat the `files` field in
`qsharp.json` for local projects their local dependencies.

Previously, if the `qsharp.json` explicitly listed any `files` , that
list would take priority and we would skip crawling the directory.

With this change: we always crawl the `src/` directory and include all
found `*.qs` files in the project. But we raise an explicit error to the
user if any of those files are missing from the `files` list.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Creating a new .qs file in a project folder where the qsharp.json has files list triggers panic
5 participants