Re-do error handling #12

sunshowers · 2020-07-08T02:35:22Z

Remove panics caused by file load or parsing errors.

This requires a new data model for errors, with a couple of new types.

Fixes #11.

(Could you do a new release once this is accepted? Thanks!)

Remove panics caused by file load or parsing errors. This requires a new data model for errors, with a couple of new types.

sunshowers · 2020-07-08T02:36:05Z

src/lib.rs

+                );
+                assert_eq!(error.module_name(), "invalid");
+                assert_eq!(error.src_span().start().line, 2);
+                assert_eq!(error.src_span().start().column, 0);
+                assert_eq!(error.src_span().end().line, 2);
+                assert_eq!(error.src_span().end().column, 12);
+                assert_eq!(error.path(), Path::new("src/invalid.rs"));
+                match error.kind() {
+                    ErrorKind::Parse(_) => {}


This would previously panic but now adds an error to the list.

Love the added detail in the test cases. Do you think it's worth having separate test cases for the different errors that can arise when accessing the root file?

sunshowers · 2020-07-08T02:46:32Z

Ahh, looks like CI is failing on Rust 1.32. Would you be willing to consider bumping the MSRV to 1.40 (for #[non_exhaustive]) given that this is a breaking change anyway?

TedDriggs

Thanks for doing this, and I love the high-level approach. There are a few minor nits and some naming questions, but I think we can get this merged and shipped this week.

TedDriggs · 2020-07-08T13:31:02Z

src/lib.rs

@@ -61,8 +63,8 @@ impl InlinerBuilder {
        self
    }

-    /// Configures whether unexpanded modules (due to a missing file) will lead
-    /// to an `Err` return value or not.
+    /// Configures whether unexpanded modules (due to missing files or invalid Rust sourcd code)


Suggested change

/// Configures whether unexpanded modules (due to missing files or invalid Rust sourcd code)

/// Configures whether unexpanded modules (due to missing files or invalid Rust source code)

Nit: Fixed typo.

TedDriggs · 2020-07-08T13:44:18Z

src/lib.rs

@@ -88,42 +90,167 @@ impl InlinerBuilder {
        };
        let result =
            Visitor::<R>::with_resolver(src_file, self.root, errors.as_mut(), Cow::Owned(resolver))
-                .visit();
+                .visit()
+                .map_err(|kind| Error::Initial(kind))?;


Suggested change

.map_err(|kind| Error::Initial(kind))?;

.map_err(Error::Initial)?;

Minor change to keep clippy happy.

TedDriggs · 2020-07-08T13:52:05Z

src/lib.rs

+    /// An error happened while reading or parsing the initial file.
+    ///
+    /// For example, the initial file wasn't found or wasn't a valid Rust file.
+    Initial(ErrorKind),


I keep tripping over the names of these two enum variants. How would you feel about this variant being called Root?

TedDriggs · 2020-07-08T13:53:28Z

src/lib.rs

+    Parse(syn::Error),
+}
+
+impl From<std::io::Error> for ErrorKind {


Aside: I have a crate, from_variants that generates these automatically. Might be overkill for just two of them.

TedDriggs · 2020-07-08T13:56:13Z

src/lib.rs

+                );
+                assert_eq!(error.module_name(), "invalid");
+                assert_eq!(error.src_span().start().line, 2);
+                assert_eq!(error.src_span().start().column, 0);
+                assert_eq!(error.src_span().end().line, 2);
+                assert_eq!(error.src_span().end().column, 12);
+                assert_eq!(error.path(), Path::new("src/invalid.rs"));
+                match error.kind() {
+                    ErrorKind::Parse(_) => {}


Love the added detail in the test cases. Do you think it's worth having separate test cases for the different errors that can arise when accessing the root file?

TedDriggs · 2020-07-08T14:03:21Z

src/lib.rs

-    _private: (),
+/// An error that was encountered while reading, parsing or inlining a module.
+#[derive(Debug)]
+#[non_exhaustive]


If we omit the use of this attribute, should this otherwise work on 1.32.0? Despite having written multiple proc-macro crates, I've never really gotten my head around how rustc versions and proc-macros interact, so I'm not sure if bumping the min-version of this crate would lead to its dependent crates also having to bump their rustc version.

If removing this attribute lets us keep 1.32.0 at the expense of requiring a minor version bump for the next error case that comes along, I think that'd be an acceptable tradeoff. I think you've been thoughtful enough in covering the cases that I don't think it's likely we'll need more any time soon.

TedDriggs · 2020-07-08T14:04:52Z

src/lib.rs

-    /// Configures whether unexpanded modules (due to a missing file) will lead
-    /// to an `Err` return value or not.
+    /// Configures whether unexpanded modules (due to missing files or invalid Rust sourcd code)
+    /// will lead to an `Err` return value or not.
    ///
    /// Default: `false`.
    pub fn error_not_found(&mut self, error_not_found: bool) -> &mut Self {


Should this be changed to report_errors or something? It's not just about cases where the file isn't found.

TedDriggs · 2020-07-08T14:06:33Z

src/lib.rs

+
+    /// Returns the `Span` (including line and column information) in the source path that caused
+    /// `self.path()` to be included.
+    pub fn src_span(&self) -> proc_macro2::Span {


Suggested change

pub fn src_span(&self) -> proc_macro2::Span {

pub fn src_span(&self) -> Span {

If you were deliberately fully qualifying the path here for rustdoc's benefit, feel free to ignore this suggestion. Otherwise, since we have the type locally imported it seems cleaner not to repeat the crate name.

TedDriggs · 2020-07-08T14:07:34Z

src/visitor.rs

@@ -18,33 +18,38 @@ pub(crate) struct Visitor<'a, R: Clone> {
    /// a direct file-system dependency so the expander can be tested.
    resolver: Cow<'a, R>,
    /// A log of module items that weren't expanded.
-    not_found_log: Option<&'a mut Vec<(String, SourceLocation)>>
+    inline_error_log: Option<&'a mut Vec<InlineError>>,


Nit: Should this just be called error_log?

TedDriggs · 2020-07-08T14:11:16Z

src/lib.rs

-    InlinerBuilder::default()
-        .parse_and_inline_modules(src_file)
-        .unwrap()
+pub fn parse_and_inline_modules(src_file: &std::path::Path) -> Result<syn::File, Error> {


Returning syn::File instead of a result was based on the signature from dtolnay/request-for-implementation#6. Since I operate on the belief that @dtolnay knows a lot more than me and has good reasons for most of his suggestions, how would you feel about leaving this signature as-is and adding try_parse_and_inline_modules or something as a sibling public function?

Found the reason why this is infallible here.

I'm leaning towards leaving this function as-is, but updating its documentation and/or the crate documentation to make clearer that there's a way to build an inliner that will preserve/return errors found during inlining.

TedDriggs · 2020-07-08T14:22:29Z

src/lib.rs

+
+    /// The initial file was successfully loaded, but some errors happened while attempting to
+    /// inline modules.
+    Inline(Vec<InlineError>),


Based on this conversation, I wonder if this should be Error::Incomplete(IncompleteInlining) where IncompleteInlining is:

#[non_exhaustive] pub struct IncompleteInlining { pub file: syn::File, pub errors: Vec<InlineError>, }

That way a caller can still get whatever succeeded and have the errors.

* Preserve signature of parse_and_inline_modules * Add IncompleteInlining to report errors during recursion * Improve doc comments for parse_and_inline_modules Relates to #11 Amends #12

sunshowers added 2 commits July 7, 2020 13:20

Formatting

323d94e

Re-do error handling

a6b902c

Remove panics caused by file load or parsing errors. This requires a new data model for errors, with a couple of new types.

sunshowers commented Jul 8, 2020

View reviewed changes

TedDriggs requested changes Jul 8, 2020

View reviewed changes

TedDriggs reviewed Jul 8, 2020

View reviewed changes

TedDriggs mentioned this pull request Jul 8, 2020

Expose errors caused by invalid Rust syntax without panics #13

Merged

TedDriggs merged commit a6b902c into TedDriggs:master Jul 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-do error handling #12

Re-do error handling #12

sunshowers commented Jul 8, 2020 •

edited

Loading

sunshowers Jul 8, 2020

TedDriggs Jul 8, 2020

sunshowers commented Jul 8, 2020

TedDriggs left a comment

TedDriggs Jul 8, 2020

TedDriggs Jul 8, 2020

TedDriggs Jul 8, 2020

TedDriggs Jul 8, 2020

TedDriggs Jul 8, 2020

TedDriggs Jul 8, 2020

TedDriggs Jul 8, 2020

TedDriggs Jul 8, 2020

TedDriggs Jul 8, 2020

TedDriggs Jul 8, 2020

TedDriggs Jul 8, 2020

TedDriggs Jul 8, 2020

	/// Configures whether unexpanded modules (due to missing files or invalid Rust sourcd code)
	/// Configures whether unexpanded modules (due to missing files or invalid Rust source code)

	.map_err(\|kind\| Error::Initial(kind))?;
	.map_err(Error::Initial)?;

	pub fn src_span(&self) -> proc_macro2::Span {
	pub fn src_span(&self) -> Span {

Re-do error handling #12

Re-do error handling #12

Conversation

sunshowers commented Jul 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunshowers commented Jul 8, 2020

TedDriggs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunshowers commented Jul 8, 2020 •

edited

Loading