Skip to content

Commit 789b302

Browse files
[mp-alt] Add initial simple validation + errors (#21755)
## Description This PR adds a manifest error type and support for codespan reporting via `serde_spanned`. It also adds the first two simple validations for package name and editions. ## Test plan Updated existing tests. --- ## Release notes Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] gRPC: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK:
1 parent f47ea4e commit 789b302

File tree

45 files changed

+293
-108
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

45 files changed

+293
-108
lines changed

Cargo.lock

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

external-crates/move/Cargo.lock

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

external-crates/move/crates/move-package-alt/Cargo.toml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@ serde_spanned.workspace = true
1313
toml.workspace = true
1414
anyhow.workspace = true
1515
derive-where.workspace = true
16+
thiserror.workspace = true
17+
codespan-reporting.workspace = true
1618

1719
[dev-dependencies]
1820
move-command-line-common.workspace = true

external-crates/move/crates/move-package-alt/src/dependency/local.rs

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,6 @@ use std::path::{Path, PathBuf};
99
use serde::{Deserialize, Serialize};
1010
use serde_spanned::Spanned;
1111

12-
use crate::errors::Located;
13-
1412
#[derive(Debug, Serialize, Deserialize, Clone)]
1513
pub struct LocalDependency {
1614
/// The path on the filesystem, relative to the location of the containing file (which is

external-crates/move/crates/move-package-alt/src/errors.rs

Lines changed: 0 additions & 28 deletions
This file was deleted.
Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
// Copyright (c) The Diem Core Contributors
2+
// Copyright (c) The Move Contributors
3+
// SPDX-License-Identifier: Apache-2.0
4+
5+
use std::{ops::Range, path::PathBuf};
6+
7+
use codespan_reporting::{
8+
diagnostic::{Diagnostic, Label},
9+
files::SimpleFiles,
10+
term::{
11+
self,
12+
termcolor::{ColorChoice, StandardStream},
13+
},
14+
};
15+
use thiserror::Error;
16+
17+
#[derive(Error, Debug)]
18+
#[error("Invalid manifest: {kind}")]
19+
pub struct ManifestError {
20+
pub kind: ManifestErrorKind,
21+
pub span: Option<Range<usize>>,
22+
pub path: PathBuf,
23+
pub src: String,
24+
}
25+
26+
#[derive(Error, Debug)]
27+
pub enum ManifestErrorKind {
28+
#[error("package name cannot be empty")]
29+
EmptyPackageName,
30+
#[error("unsupported edition '{edition}', expected one of '{valid}'")]
31+
InvalidEdition { edition: String, valid: String },
32+
}
33+
34+
impl ManifestError {
35+
/// Convert this error into a codespan Diagnostic
36+
pub fn to_diagnostic(&self) -> Diagnostic<usize> {
37+
let (file_id, span) = self.span_info();
38+
Diagnostic::error()
39+
.with_message(self.kind.to_string())
40+
.with_labels(vec![Label::primary(file_id, span.unwrap_or_default())])
41+
}
42+
43+
/// Get the file ID and span for this error
44+
fn span_info(&self) -> (usize, Option<Range<usize>>) {
45+
// In a real implementation, we'd want to cache the SimpleFiles instance
46+
// and reuse it across multiple errors
47+
let mut files = SimpleFiles::new();
48+
let file_id = files.add(self.path.display().to_string(), self.src.clone());
49+
(file_id, self.span.clone())
50+
}
51+
52+
/// Emit this error to stderr
53+
pub fn emit(&self) -> Result<(), codespan_reporting::files::Error> {
54+
let mut files = SimpleFiles::new();
55+
let file_id = files.add(self.path.display().to_string(), self.src.clone());
56+
57+
let writer = StandardStream::stderr(ColorChoice::Always);
58+
let config = term::Config {
59+
display_style: term::DisplayStyle::Rich,
60+
chars: term::Chars::ascii(),
61+
..Default::default()
62+
};
63+
64+
let diagnostic = self.to_diagnostic();
65+
let e = term::emit(&mut writer.lock(), &config, &files, &diagnostic);
66+
e
67+
}
68+
}
Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
// Copyright (c) The Diem Core Contributors
2+
// Copyright (c) The Move Contributors
3+
// SPDX-License-Identifier: Apache-2.0
4+
5+
mod manifest_error;
6+
pub use manifest_error::ManifestError;
7+
pub use manifest_error::ManifestErrorKind;
8+
9+
use std::{ops::Range, path::PathBuf};
10+
11+
use codespan_reporting::diagnostic::Diagnostic;
12+
use serde::{Deserialize, Serialize};
13+
use thiserror::Error;
14+
15+
/// Result type for package operations
16+
pub type PackageResult<T> = Result<T, PackageError>;
17+
18+
/// The main error type for package-related operations
19+
#[derive(Error, Debug)]
20+
pub enum PackageError {
21+
#[error(transparent)]
22+
Codespan(#[from] codespan_reporting::files::Error),
23+
24+
#[error(transparent)]
25+
Io(#[from] std::io::Error),
26+
27+
#[error(transparent)]
28+
Manifest(#[from] ManifestError),
29+
30+
#[error(transparent)]
31+
Other(#[from] anyhow::Error),
32+
33+
#[error(transparent)]
34+
Toml(#[from] toml_edit::de::Error),
35+
}
36+
37+
impl PackageError {
38+
pub fn to_diagnostic(&self) -> Diagnostic<usize> {
39+
match self {
40+
Self::Manifest(err) => err.to_diagnostic(),
41+
_ => Diagnostic::error()
42+
.with_message(self.to_string())
43+
.with_labels(vec![]),
44+
}
45+
}
46+
47+
pub fn emit(&self) -> Result<(), codespan_reporting::files::Error> {
48+
match self {
49+
Self::Manifest(err) => err.emit(),
50+
_ => Ok(()),
51+
}
52+
}
53+
}

external-crates/move/crates/move-package-alt/src/package/manifest.rs

Lines changed: 45 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,14 +9,18 @@ use std::{
99

1010
use derive_where::derive_where;
1111
use serde::{Deserialize, Serialize};
12+
use serde_spanned::Spanned;
1213

1314
use crate::{
1415
dependency::ManifestDependencyInfo,
16+
errors::{ManifestError, ManifestErrorKind, PackageResult},
1517
flavor::{MoveFlavor, Vanilla},
1618
};
1719

1820
use super::*;
1921

22+
const ALLOWED_EDITIONS: &[&str] = &["2024", "2024.beta", "legacy"];
23+
2024
// Note: [Manifest] objects are immutable and should not implement [serde::Serialize]; any tool
2125
// writing these files should use [toml_edit] to set / preserve the formatting, since these are
2226
// user-editable files
@@ -36,8 +40,8 @@ pub struct Manifest<F: MoveFlavor> {
3640
#[derive(Debug, Deserialize)]
3741
#[serde(bound = "")]
3842
struct PackageMetadata<F: MoveFlavor> {
39-
name: PackageName,
40-
edition: String,
43+
name: Spanned<PackageName>,
44+
edition: Spanned<String>,
4145

4246
#[serde(flatten)]
4347
metadata: F::PackageMetadata,
@@ -72,9 +76,45 @@ struct ManifestDependencyOverride<F: MoveFlavor> {
7276
}
7377

7478
impl<F: MoveFlavor> Manifest<F> {
75-
pub fn read_from(path: impl AsRef<Path>) -> anyhow::Result<Self> {
76-
let contents = std::fs::read_to_string(path)?;
77-
Ok(toml_edit::de::from_str(&contents)?)
79+
pub fn read_from(path: impl AsRef<Path>) -> PackageResult<Self> {
80+
let contents = std::fs::read_to_string(&path)?;
81+
82+
let manifest: Self = toml_edit::de::from_str(&contents)?;
83+
manifest.validate_manifest(&path, &contents)?;
84+
85+
Ok(manifest)
86+
}
87+
88+
/// Validate the manifest contents, after deserialization.
89+
pub fn validate_manifest(&self, path: impl AsRef<Path>, contents: &str) -> PackageResult<()> {
90+
// Validate package name
91+
if self.package.name.get_ref().is_empty() {
92+
let err = ManifestError {
93+
kind: ManifestErrorKind::EmptyPackageName,
94+
span: Some(self.package.name.span()),
95+
path: path.as_ref().to_path_buf(),
96+
src: contents.to_string(),
97+
};
98+
err.emit()?;
99+
return Err(err.into());
100+
}
101+
102+
// Validate edition
103+
if !ALLOWED_EDITIONS.contains(&self.package.edition.get_ref().as_str()) {
104+
let err = ManifestError {
105+
kind: ManifestErrorKind::InvalidEdition {
106+
edition: self.package.edition.get_ref().clone(),
107+
valid: ALLOWED_EDITIONS.join(", ").to_string(),
108+
},
109+
span: Some(self.package.edition.span()),
110+
path: path.as_ref().to_path_buf(),
111+
src: contents.to_string(),
112+
};
113+
err.emit()?;
114+
return Err(err.into());
115+
}
116+
117+
Ok(())
78118
}
79119

80120
fn write_template(path: impl AsRef<Path>, name: &PackageName) -> anyhow::Result<()> {

external-crates/move/crates/move-package-alt/tests/data/manifest_parsing_invalid/basic_invalid_environment/Move.snap

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
---
2-
source: crates/move-package-alt/tests/manifest_parsing_tests.rs
2+
source: crates/move-package-alt/tests/parsing_tests.rs
33
---
4-
TOML parse error at line 8, column 11
4+
error: TOML parse error at line 8, column 11
55
|
66
8 | mainnet = 9123
77
| ^^^^

external-crates/move/crates/move-package-alt/tests/data/manifest_parsing_invalid/basic_invalid_environment1/Move.snap

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
---
2-
source: crates/move-package-alt/tests/manifest_parsing_tests.rs
2+
source: crates/move-package-alt/tests/parsing_tests.rs
33
---
4-
TOML parse error at line 8, column 11
4+
error: TOML parse error at line 8, column 11
55
|
66
8 | mainnet = true
77
| ^^^^

0 commit comments

Comments
 (0)