Parse manifest in Rust instead of Python (#1330)

This PR regains some rust real-state. It delegates the parsing of the manifest in `pip/qsharp/_qsharp.py/init()` to rust, avoiding duplicate manifest-parsing logic in the project. This change is needed to unblock #1313 --------- Co-authored-by: Mine Starks <[email protected]>
microsoft · Apr 23, 2024 · 28817e4 · 28817e4
1 parent f967538
commit 28817e4
Show file tree

Hide file tree

Showing 10 changed files with 63 additions and 66 deletions.
diff --git a/compiler/qsc_linter/src/linter.rs b/compiler/qsc_linter/src/linter.rs
@@ -9,7 +9,7 @@ use crate::lints::{ast::AstLint, hir::HirLint};
 use miette::{Diagnostic, LabeledSpan};
 use qsc_data_structures::span::Span;
 use qsc_frontend::compile::CompileUnit;
-use serde::Deserialize;
+use serde::{Deserialize, Serialize};
 use std::fmt::Display;
 
 /// The entry point to the linter. It takes a [`qsc_frontend::compile::CompileUnit`]
@@ -73,7 +73,7 @@ impl Diagnostic for Lint {
 
 /// A lint level. This defines if a lint will be treated as a warning or an error,
 /// and if the lint level can be overriden by the user.
-#[derive(Debug, Clone, Copy, Deserialize)]
+#[derive(Debug, Clone, Copy, Deserialize, Serialize)]
 #[serde(rename_all = "camelCase")]
 pub enum LintLevel {
     /// The lint is effectively disabled.
@@ -101,7 +101,7 @@ impl Display for LintLevel {
 }
 
 /// End-user configuration for each lint level.
-#[derive(Debug, Clone, Deserialize)]
+#[derive(Debug, Clone, Deserialize, Serialize)]
 pub struct LintConfig {
     #[serde(rename = "lint")]
     /// Represents the lint name.
@@ -111,7 +111,7 @@ pub struct LintConfig {
 }
 
 /// Represents a lint name.
-#[derive(Debug, Clone, Copy, Deserialize)]
+#[derive(Debug, Clone, Copy, Deserialize, Serialize)]
 #[serde(untagged)]
 pub enum LintKind {
     /// AST lint name.

diff --git a/compiler/qsc_linter/src/linter/ast.rs b/compiler/qsc_linter/src/linter/ast.rs
@@ -131,9 +131,9 @@ macro_rules! declare_ast_lints {
 
     // Declare the `AstLint` enum.
     (@CONFIG_ENUM $($lint_name:ident),*) => {
-        use serde::Deserialize;
+        use serde::{Deserialize, Serialize};
 
-        #[derive(Debug, Clone, Copy, Deserialize)]
+        #[derive(Debug, Clone, Copy, Deserialize, Serialize)]
         #[serde(rename_all = "camelCase")]
         pub enum AstLint {
             $($lint_name),*

diff --git a/compiler/qsc_linter/src/linter/hir.rs b/compiler/qsc_linter/src/linter/hir.rs
@@ -119,9 +119,9 @@ macro_rules! declare_hir_lints {
 
     // Declare the `HirLint` enum.
     (@CONFIG_ENUM $($lint_name:ident),*) => {
-        use serde::Deserialize;
+        use serde::{Deserialize, Serialize};
 
-        #[derive(Debug, Clone, Copy, Deserialize)]
+        #[derive(Debug, Clone, Copy, Deserialize, Serialize)]
         #[serde(rename_all = "camelCase")]
         pub enum HirLint {
             $($lint_name),*

diff --git a/compiler/qsc_project/src/manifest.rs b/compiler/qsc_project/src/manifest.rs
@@ -10,13 +10,13 @@ use std::{
 };
 
 pub use qsc_linter::LintConfig;
-use serde::Deserialize;
+use serde::{Deserialize, Serialize};
 use std::{path::PathBuf, sync::Arc};
 
 pub const MANIFEST_FILE_NAME: &str = "qsharp.json";
 
 /// A Q# manifest, used to describe project metadata.
-#[derive(Deserialize, Debug, Default, Clone)]
+#[derive(Deserialize, Serialize, Debug, Default, Clone)]
 #[serde(rename_all = "camelCase")]
 pub struct Manifest {
     pub author: Option<String>,

diff --git a/language_service/src/protocol.rs b/language_service/src/protocol.rs
@@ -3,6 +3,7 @@
 
 use qsc::line_column::Range;
 use qsc::{compile::Error, target::Profile, LanguageFeatures, PackageType};
+use qsc_project::Manifest;
 
 /// A change to the workspace configuration
 #[derive(Clone, Debug, Default, Copy)]
@@ -122,6 +123,7 @@ pub struct ParameterInformation {
 pub struct NotebookMetadata {
     pub target_profile: Option<Profile>,
     pub language_features: LanguageFeatures,
+    pub manifest: Option<Manifest>,
 }
 
 #[derive(Debug)]

diff --git a/language_service/src/state.rs b/language_service/src/state.rs
@@ -367,7 +367,7 @@ impl<'a> CompilationStateUpdater<'a> {
                 target_profile: notebook_metadata.target_profile,
                 package_type: None,
                 language_features: Some(notebook_metadata.language_features),
-                lints_config: None,
+                lints_config: notebook_metadata.manifest.map(|manifest| manifest.lints),
             };
             let configuration = merge_configurations(&notebook_configuration, &configuration);
 

diff --git a/pip/qsharp/_qsharp.py b/pip/qsharp/_qsharp.py
@@ -23,7 +23,12 @@ class Config:
     Configuration hints for the language service.
     """
 
-    def __init__(self, target_profile: TargetProfile, language_features: List[str]):
+    def __init__(
+        self,
+        target_profile: TargetProfile,
+        language_features: Optional[List[str]],
+        manifest: Optional[str],
+    ):
         if target_profile == TargetProfile.Quantinuum:
             self._config = {"targetProfile": "quantinuum"}
             warn("The Quantinuum target profile is a preview feature.")
@@ -34,6 +39,7 @@ def __init__(self, target_profile: TargetProfile, language_features: List[str]):
             self._config = {"targetProfile": "unrestricted"}
 
         self._config["languageFeatures"] = language_features
+        self._config["manifest"] = manifest
 
     def __repr__(self) -> str:
         return "Q# initialized with configuration: " + str(self._config)
@@ -55,7 +61,7 @@ def init(
     *,
     target_profile: TargetProfile = TargetProfile.Unrestricted,
     project_root: Optional[str] = None,
-    language_features: List[str] = [],
+    language_features: Optional[List[str]] = None,
 ) -> Config:
     """
     Initializes the Q# interpreter.
@@ -71,6 +77,7 @@ def init(
 
     global _interpreter
 
+    manifest_contents = None
     manifest_descriptor = None
     if project_root is not None:
         qsharp_json = join(project_root, "qsharp.json")
@@ -83,26 +90,13 @@ def init(
         manifest_descriptor["manifest_dir"] = project_root
 
         try:
-            (_, file_contents) = read_file(qsharp_json)
+            (_, manifest_contents) = read_file(qsharp_json)
+            manifest_descriptor["manifest"] = manifest_contents
         except Exception as e:
             raise QSharpError(
                 f"Error reading {qsharp_json}. qsharp.json should exist at the project root and be a valid JSON file."
             ) from e
 
-        try:
-            manifest_descriptor["manifest"] = json.loads(file_contents)
-        except Exception as e:
-            raise QSharpError(
-                f"Error parsing {qsharp_json}. qsharp.json should exist at the project root and be a valid JSON file."
-            ) from e
-
-    # if no features were passed in as an argument, use the features from the manifest.
-    # this way we prefer the features from the argument over those from the manifest.
-    if language_features == [] and manifest_descriptor != None:
-        language_features = (
-            manifest_descriptor["manifest"].get("languageFeatures") or []
-        )
-
     _interpreter = Interpreter(
         target_profile,
         language_features,
@@ -113,7 +107,7 @@ def init(
 
     # Return the configuration information to provide a hint to the
     # language service through the cell output.
-    return Config(target_profile, language_features)
+    return Config(target_profile, language_features, manifest_contents)
 
 
 def get_interpreter() -> Interpreter:

diff --git a/pip/src/interpreter.rs b/pip/src/interpreter.rs
@@ -81,22 +81,19 @@ impl FromPyObject<'_> for PyManifestDescriptor {
         let manifest_dir = get_dict_opt_string(dict, "manifest_dir")?.ok_or(
             PyException::new_err("missing key `manifest_dir` in manifest descriptor"),
         )?;
-        let manifest = dict
-            .get_item("manifest")?
-            .ok_or(PyException::new_err(
-                "missing key `manifest` in manifest descriptor",
-            ))?
-            .downcast::<PyDict>()?;
 
-        let language_features = get_dict_opt_list_string(manifest, "features")?;
+        let manifest = get_dict_opt_string(dict, "manifest")?.ok_or(PyException::new_err(
+            "missing key `manifest` in manifest descriptor",
+        ))?;
+
+        let manifest = serde_json::from_str::<Manifest>(&manifest).map_err(|_| {
+            PyErr::new::<PyException, _>(format!(
+                "Error parsing {manifest_dir}/qsharp.json . Manifest should be a valid JSON file."
+            ))
+        })?;
 
         Ok(Self(ManifestDescriptor {
-            manifest: Manifest {
-                author: get_dict_opt_string(manifest, "author")?,
-                license: get_dict_opt_string(manifest, "license")?,
-                language_features,
-                lints: vec![],
-            },
+            manifest,
             manifest_dir: manifest_dir.into(),
         }))
     }
@@ -121,7 +118,15 @@ impl Interpreter {
             TargetProfile::Base => Profile::Base,
             TargetProfile::Unrestricted => Profile::Unrestricted,
         };
-        let language_features = language_features.unwrap_or_default();
+        // If no features were passed in as an argument, use the features from the manifest.
+        // this way we prefer the features from the argument over those from the manifest.
+        let language_features: Vec<String> = match (language_features, &manifest_descriptor) {
+            (Some(language_features), _) => language_features,
+            (_, Some(manifest_descriptor)) => {
+                manifest_descriptor.0.manifest.language_features.clone()
+            }
+            (None, None) => vec![],
+        };
 
         let sources = if let Some(manifest_descriptor) = manifest_descriptor {
             let project = file_system(
@@ -607,21 +612,3 @@ fn get_dict_opt_string(dict: &PyDict, key: &str) -> PyResult<Option<String>> {
         None => None,
     })
 }
-fn get_dict_opt_list_string(dict: &PyDict, key: &str) -> PyResult<Vec<String>> {
-    let value = dict.get_item(key)?;
-    let list: &PyList = match value {
-        Some(item) => item.downcast::<PyList>()?,
-        None => return Ok(vec![]),
-    };
-    match list
-        .iter()
-        .map(|item| {
-            item.downcast::<PyString>()
-                .map(|s| s.to_string_lossy().into())
-        })
-        .collect::<std::result::Result<Vec<String>, _>>()
-    {
-        Ok(list) => Ok(list),
-        Err(e) => Err(e.into()),
-    }
-}
diff --git a/pip/tests/test_project.py b/pip/tests/test_project.py
@@ -33,7 +33,7 @@ def test_project_compile_error(qsharp) -> None:
 def test_project_bad_qsharp_json(qsharp) -> None:
     with pytest.raises(Exception) as excinfo:
         qsharp.init(project_root="/bad_qsharp_json")
-    assert str(excinfo.value).startswith("Error parsing /bad_qsharp_json/qsharp.json.")
+    assert str(excinfo.value).startswith("Error parsing /bad_qsharp_json/qsharp.json")
 
 
 def test_project_unreadable_qsharp_json(qsharp) -> None:

diff --git a/wasm/src/language_service.rs b/wasm/src/language_service.rs
@@ -12,6 +12,7 @@ use crate::{
     serializable_type,
 };
 use qsc::{self, line_column::Encoding, target::Profile, LanguageFeatures, PackageType};
+use qsc_project::Manifest;
 use qsls::protocol::DiagnosticUpdate;
 use rustc_hash::FxHashMap;
 use serde::{Deserialize, Serialize};
@@ -118,15 +119,26 @@ impl LanguageService {
     ) {
         let cells: Vec<Cell> = cells.into_iter().map(std::convert::Into::into).collect();
         let notebook_metadata: NotebookMetadata = notebook_metadata.into();
+        let manifest: Option<Manifest> = notebook_metadata
+            .manifest
+            .and_then(|manifest| serde_json::from_str(&manifest).ok());
+
+        // If no features were passed in as an argument, use the features from the manifest.
+        // this way we prefer the features from the argument over those from the manifest.
+        let language_features: Vec<String> = match (notebook_metadata.languageFeatures, &manifest) {
+            (Some(language_features), _) => language_features,
+            (_, Some(manifest)) => manifest.language_features.clone(),
+            (None, None) => vec![],
+        };
+
         self.0.update_notebook_document(
             notebook_uri,
             qsls::protocol::NotebookMetadata {
                 target_profile: notebook_metadata
                     .targetProfile
                     .map(|s| Profile::from_str(&s).expect("invalid target profile")),
-                language_features: LanguageFeatures::from_iter(
-                    notebook_metadata.languageFeatures.unwrap_or_default(),
-                ),
+                language_features: LanguageFeatures::from_iter(language_features),
+                manifest,
             },
             cells
                 .iter()
@@ -486,11 +498,13 @@ serializable_type! {
     NotebookMetadata,
     {
         pub targetProfile: Option<String>,
-        pub languageFeatures: Option<Vec<String>>
+        pub languageFeatures: Option<Vec<String>>,
+        pub manifest: Option<String>,
     },
     r#"export interface INotebookMetadata {
         targetProfile?: "base" | "quantinuum" | "unrestricted";
         languageFeatures?: "v2-preview-syntax"[];
+        manifest?: string;
     }"#,
     INotebookMetadata
 }