fix language service panic when file is under the project folder but not in the files list

Author: minestarks

compiler/qsc_project/src/project.rs

@@ -3,7 +3,7 @@
 
 use crate::{
     manifest::{GitHubRef, PackageType},
-    Manifest, ManifestDescriptor, PackageRef,
+    Manifest, PackageRef,
 };
 use async_trait::async_trait;
 use futures::FutureExt;
@@ -129,6 +129,11 @@ pub enum Error {
     #[error("Error fetching from GitHub: {0}")]
     #[diagnostic(code("Qsc.Project.GitHub"))]
     GitHub(String),
+
+    #[error("File {0} is not listed in the `files` field of the manifest")]
+    #[help("To avoid unexpected behavior, add this file to the `files` field in the `qsharp.json` manifest")]
+    #[diagnostic(code("Qsc.Project.DocumentNotInProject"))]
+    DocumentNotInProject(String),
 }
 
 impl Error {
@@ -137,6 +142,7 @@ impl Error {
     pub fn path(&self) -> Option<&String> {
         match self {
             Error::GitHubManifestParse { path, .. }
+            | Error::DocumentNotInProject(path)
             | Error::NoSrcDir { path }
             | Error::ManifestParse { path, .. } => Some(path),
             // Note we don't return the path for `FileSystem` errors,
@@ -182,10 +188,7 @@ pub trait FileSystemAsync {
     ) -> miette::Result<Arc<str>>;
 
     /// Given an initial path, fetch files matching <initial_path>/**/*.qs
-    async fn collect_project_sources(
-        &self,
-        initial_path: &Path,
-    ) -> ProjectResult<Vec<Self::Entry>> {
+    async fn collect_project_sources(&self, initial_path: &Path) -> ProjectResult<Vec<PathBuf>> {
         let listing = self
             .list_directory(initial_path)
             .await
@@ -210,7 +213,7 @@ pub trait FileSystemAsync {
     async fn collect_project_sources_inner(
         &self,
         initial_path: &Path,
-    ) -> ProjectResult<Vec<Self::Entry>> {
+    ) -> ProjectResult<Vec<PathBuf>> {
         let listing = self
             .list_directory(initial_path)
             .await
@@ -221,7 +224,7 @@ pub trait FileSystemAsync {
         let mut files = vec![];
         for item in filter_hidden_files(listing.into_iter()) {
             match item.entry_type() {
-                Ok(EntryType::File) if item.entry_extension() == "qs" => files.push(item),
+                Ok(EntryType::File) if item.entry_extension() == "qs" => files.push(item.path()),
                 Ok(EntryType::Folder) => {
                     files.append(&mut self.collect_project_sources_inner(&item.path()).await?);
                 }
@@ -231,8 +234,64 @@ pub trait FileSystemAsync {
         Ok(files)
     }
 
+    async fn collect_sources_from_files_field(
+        &self,
+        project_path: &Path,
+        manifest: &Manifest,
+    ) -> ProjectResult<Vec<PathBuf>> {
+        let mut v = vec![];
+        for file in &manifest.files {
+            v.push(
+                self.resolve_path(project_path, Path::new(&file))
+                    .await
+                    .map_err(|e| Error::FileSystem {
+                        about_path: project_path.to_string_lossy().to_string(),
+                        error: e.to_string(),
+                    })?,
+            );
+        }
+        Ok(v)
+    }
+
+    fn validate(
+        &self,
+        qs_files: &mut Vec<PathBuf>,
+        listed_files: &mut Vec<PathBuf>,
+    ) -> Result<(), Vec<Error>> {
+        qs_files.sort();
+        listed_files.sort();
+
+        // If the `files` field exists in the manifest, validate it includes
+        // all the files in the `src` directory.
+        // how do I subtract one sorted vector from another
+        let mut difference = qs_files.clone();
+        let mut iter2 = listed_files.iter().peekable();
+
+        difference.retain(|item| {
+            while let Some(&next) = iter2.peek() {
+                if next < item {
+                    iter2.next();
+                } else {
+                    break;
+                }
+            }
+            iter2.peek() != Some(&item)
+        });
+
+        if !difference.is_empty() {
+            return Err(difference
+                .iter()
+                .map(|p| Error::DocumentNotInProject(p.to_string_lossy().to_string()))
+                .collect());
+        }
+        Ok(())
+    }
+
     /// Given a directory, loads the project sources
     /// and the sources for all its dependencies.
+    ///
+    /// Any errors that didn't block project load are contained in the
+    /// `errors` field of the returned `Project`.
     async fn load_project(
         &self,
         directory: &Path,
@@ -243,15 +302,15 @@ pub trait FileSystemAsync {
             .await
             .map_err(|e| vec![e])?;
 
-        let root = self
-            .read_local_manifest_and_sources(directory)
-            .await
-            .map_err(|e| vec![e])?;
-
         let mut errors = vec![];
         let mut packages = FxHashMap::default();
         let mut stack = vec![];
 
+        let root = self
+            .read_local_manifest_and_sources(directory, &mut errors)
+            .await
+            .map_err(|e| vec![e])?;
+
         let root_path = directory.to_string_lossy().to_string();
         let root_ref = PackageRef::Path { path: root_path };
 
@@ -321,41 +380,33 @@ pub trait FileSystemAsync {
 
     /// Load the sources for a single package at the given directory. Also load its
     /// dependency information but don't recurse into dependencies yet.
+    ///
+    /// Any errors that didn't block project load are accumulated into the `errors` vector.
     async fn read_local_manifest_and_sources(
         &self,
-        directory: &Path,
+        manifest_dir: &Path,
+        errors: &mut Vec<Error>,
     ) -> ProjectResult<PackageInfo> {
-        let manifest = self.parse_manifest_in_dir(directory).await?;
-
-        let manifest = ManifestDescriptor {
-            manifest_dir: directory.to_path_buf(),
-            manifest,
-        };
-
-        let project_path = manifest.manifest_dir.clone();
-
-        // If the `files` field exists in the manifest, prefer that.
-        // Otherwise, collect all files in the project directory.
-        let qs_files: Vec<PathBuf> = if manifest.manifest.files.is_empty() {
-            let qs_files = self.collect_project_sources(&project_path).await?;
-            qs_files.into_iter().map(|file| file.path()).collect()
-        } else {
-            let mut v = vec![];
-            for file in manifest.manifest.files {
-                v.push(
-                    self.resolve_path(&project_path, Path::new(&file))
-                        .await
-                        .map_err(|e| Error::FileSystem {
-                            about_path: project_path.to_string_lossy().to_string(),
-                            error: e.to_string(),
-                        })?,
-                );
-            }
-            v
-        };
+        let manifest = self.parse_manifest_in_dir(manifest_dir).await?;
+
+        // All the *.qs files under src/
+        let mut all_qs_files = self.collect_project_sources(manifest_dir).await?;
+
+        // Files explicitly listed in the `files` field of the manifest
+        let mut listed_files = self
+            .collect_sources_from_files_field(manifest_dir, &manifest)
+            .await?;
+
+        if !listed_files.is_empty() {
+            errors.extend(
+                self.validate(&mut all_qs_files, &mut listed_files)
+                    .err()
+                    .unwrap_or_default(),
+            );
+        }
 
-        let mut sources = Vec::with_capacity(qs_files.len());
-        for path in qs_files {
+        let mut sources = Vec::with_capacity(all_qs_files.len());
+        for path in all_qs_files {
             sources.push(self.read_file(&path).await.map_err(|e| Error::FileSystem {
                 about_path: path.to_string_lossy().to_string(),
                 error: e.to_string(),
@@ -367,13 +418,13 @@ pub trait FileSystemAsync {
         // For any local dependencies, convert relative paths to absolute,
         // so that multiple references to the same package, from different packages,
         // get merged correctly.
-        for (alias, mut dep) in manifest.manifest.dependencies {
+        for (alias, mut dep) in manifest.dependencies {
             if let PackageRef::Path { path: dep_path } = &mut dep {
                 *dep_path = self
-                    .resolve_path(&project_path, &PathBuf::from(dep_path.clone()))
+                    .resolve_path(manifest_dir, &PathBuf::from(dep_path.clone()))
                     .await
                     .map_err(|e| Error::FileSystem {
-                        about_path: project_path.to_string_lossy().to_string(),
+                        about_path: manifest_dir.to_string_lossy().to_string(),
                         error: e.to_string(),
                     })?
                     .to_string_lossy()
@@ -384,9 +435,9 @@ pub trait FileSystemAsync {
 
         Ok(PackageInfo {
             sources,
-            language_features: LanguageFeatures::from_iter(&manifest.manifest.language_features),
+            language_features: LanguageFeatures::from_iter(manifest.language_features),
             dependencies,
-            package_type: manifest.manifest.package_type,
+            package_type: manifest.package_type,
         })
     }
 
@@ -477,6 +528,7 @@ pub trait FileSystemAsync {
         global_cache: &RefCell<PackageCache>,
         key: PackageKey,
         this_pkg: &PackageRef,
+        errors: &mut Vec<Error>,
     ) -> ProjectResult<PackageInfo> {
         match this_pkg {
             PackageRef::GitHub { github } => {
@@ -499,7 +551,7 @@ pub trait FileSystemAsync {
                 // editing experience as intuitive as possible. This may change if we start
                 // hitting perf issues, but careful consideration is needed into when to
                 // invalidate the cache.
-                self.read_local_manifest_and_sources(PathBuf::from(path.clone()).as_path())
+                self.read_local_manifest_and_sources(PathBuf::from(path.clone()).as_path(), errors)
                     .await
             }
         }
@@ -535,7 +587,7 @@ pub trait FileSystemAsync {
             }
 
             let dep_result = self
-                .read_manifest_and_sources(global_cache, dep_key.clone(), &dependency)
+                .read_manifest_and_sources(global_cache, dep_key.clone(), &dependency, errors)
                 .await;
 
             match dep_result {

compiler/qsc_project/src/tests.rs

@@ -426,6 +426,10 @@ fn explicit_files_list() {
                                 "explicit_files_list/src/Main.qs",
                                 "namespace Dependency {\n    function LibraryFn() : Unit {\n    }\n}\n",
                             ),
+                            (
+                                "explicit_files_list/src/NotIncluded.qs",
+                                "namespace Dependency {\n    function LibraryFn() : Unit {\n    }\n}\n",
+                            ),
                         ],
                         language_features: LanguageFeatures(
                             0,
@@ -436,7 +440,11 @@ fn explicit_files_list() {
                     packages: {},
                 },
                 lints: [],
-                errors: [],
+                errors: [
+                    DocumentNotInProject(
+                        "explicit_files_list/src/NotIncluded.qs",
+                    ),
+                ],
             }"#]],
     );
 }

compiler/qsc_project/src/tests/harness.rs

@@ -59,7 +59,8 @@ fn normalize(project: &mut Project, root_path: &Path) {
         match err {
             Error::NoSrcDir { path }
             | Error::ManifestParse { path, .. }
-            | Error::GitHubManifestParse { path, .. } => {
+            | Error::GitHubManifestParse { path, .. }
+            | Error::DocumentNotInProject(path) => {
                 let mut str = std::mem::take(path).into();
                 remove_absolute_path_prefix(&mut str, root_path);
                 *path = str.to_string();
@@ -74,13 +75,11 @@ fn normalize(project: &mut Project, root_path: &Path) {
                 *error = "REPLACED".to_string();
             }
             Error::Circular(s1, s2) | Error::GitHubToLocal(s1, s2) => {
-                // These errors contain absolute paths which don't work well in test output
+                // these strings can contain mangled absolute paths so we can't fix them
                 *s1 = "REPLACED".to_string();
                 *s2 = "REPLACED".to_string();
             }
-            Error::GitHub(s) => {
-                *s = "REPLACED".to_string();
-            }
+            Error::GitHub(_) => {}
         }
     }
 }