[naga] Warn, rather than error, on unreachable statements

Fixes #7536

Author: andyleiserson

naga-cli/src/bin/naga.rs

@@ -4,6 +4,8 @@ use anyhow::{anyhow, Context as _};
 use std::fs;
 use std::{error::Error, fmt, io::Read, path::Path, str::FromStr};
 
+use naga::Diagnostic;
+
 /// Translate shaders to different formats.
 #[derive(argh::FromArgs, Debug, Clone)]
 struct Args {

naga/src/error.rs

@@ -1,6 +1,8 @@
 use alloc::{boxed::Box, string::String, vec::Vec};
 use core::{error::Error, fmt};
 
+use crate::span::Diagnostic;
+
 #[derive(Clone, Debug)]
 pub struct ShaderError<E> {
     /// The source code of the shader.

naga/src/lib.rs

@@ -134,7 +134,7 @@ pub mod valid;
 use alloc::string::String;
 
 pub use crate::arena::{Arena, Handle, Range, UniqueArena};
-pub use crate::span::{SourceLocation, Span, SpanContext, WithSpan};
+pub use crate::span::{Diagnostic, SourceLocation, Span, SpanContext, WithSpan};
 
 // TODO: Eliminate this re-export and migrate uses of `crate::Foo` to `use crate::ir; ir::Foo`.
 pub use ir::*;

naga/src/span.rs

@@ -1,5 +1,6 @@
 use alloc::{
     borrow::ToOwned,
+    boxed::Box,
     format,
     string::{String, ToString},
     vec::Vec,
@@ -140,6 +141,20 @@ pub struct WithSpan<E> {
     spans: Vec<SpanContext>,
 }
 
+/// Variant of [`WithSpan`] that holds a boxed [`Error`].
+///
+/// This is currently used to hold warnings from validation, and since we don't
+/// have severity information in [`WithSpan`] or in the error itself, the
+/// implementations of [`Diagnostic`] assume that any [`WithSpan`] is a
+/// [`codespan_reporting::diagnostic::Severity::Error`] and any
+/// [`WithSpanBoxed`] is a
+/// [`codespan_reporting::diagnostic::Severity::Warning`].
+#[derive(Debug)]
+pub struct WithSpanBoxed {
+    inner: Box<dyn Error>,
+    spans: Vec<SpanContext>,
+}
+
 impl<E> fmt::Display for WithSpan<E>
 where
     E: fmt::Display,
@@ -237,6 +252,17 @@ impl<E> WithSpan<E> {
         res
     }
 
+    /// Convert this [`WithSpan`] into a [`WithSpanBoxed`].
+    pub fn boxed(self) -> WithSpanBoxed
+    where
+        E: Error + 'static,
+    {
+        WithSpanBoxed {
+            inner: Box::new(self.inner),
+            spans: self.spans,
+        }
+    }
+
     /// Return a [`SourceLocation`] for our first span, if we have one.
     pub fn location(&self, source: &str) -> Option<SourceLocation> {
         if self.spans.is_empty() || source.is_empty() {
@@ -245,46 +271,61 @@ impl<E> WithSpan<E> {
 
         Some(self.spans[0].0.location(source))
     }
+}
 
-    pub(crate) fn diagnostic(&self) -> codespan_reporting::diagnostic::Diagnostic<()>
-    where
-        E: Error,
-    {
-        use codespan_reporting::diagnostic::{Diagnostic, Label};
-        let diagnostic = Diagnostic::error()
-            .with_message(self.inner.to_string())
-            .with_labels(
-                self.spans()
-                    .map(|&(span, ref desc)| {
-                        Label::primary((), span.to_range().unwrap()).with_message(desc.to_owned())
-                    })
-                    .collect(),
-            )
-            .with_notes({
-                let mut notes = Vec::new();
-                let mut source: &dyn Error = &self.inner;
-                while let Some(next) = Error::source(source) {
-                    notes.push(next.to_string());
-                    source = next;
-                }
-                notes
-            });
-        diagnostic
+fn diagnostic(
+    severity: codespan_reporting::diagnostic::Severity,
+    error: &dyn Error,
+    spans: &[SpanContext],
+) -> codespan_reporting::diagnostic::Diagnostic<()> {
+    use codespan_reporting::diagnostic::{Diagnostic, Label};
+    let diagnostic = Diagnostic::new(severity)
+        .with_message(error.to_string())
+        .with_labels(
+            spans
+                .iter()
+                .map(|&(span, ref desc)| {
+                    Label::primary((), span.to_range().unwrap()).with_message(desc.to_owned())
+                })
+                .collect(),
+        )
+        .with_notes({
+            let mut notes = Vec::new();
+            let mut source = error;
+            while let Some(next) = Error::source(source) {
+                notes.push(next.to_string());
+                source = next;
+            }
+            notes
+        });
+    diagnostic
+}
+
+impl<E: Error> Diagnostic for WithSpan<E> {
+    fn diagnostic(&self) -> codespan_reporting::diagnostic::Diagnostic<()> {
+        use codespan_reporting::diagnostic::Severity;
+        diagnostic(Severity::Error, &self.inner, &self.spans)
     }
+}
+
+impl Diagnostic for WithSpanBoxed {
+    fn diagnostic(&self) -> codespan_reporting::diagnostic::Diagnostic<()> {
+        /// We assume that `self` is a warning. See documentation for [`WithSpanBoxed`].
+        use codespan_reporting::diagnostic::Severity;
+        diagnostic(Severity::Warning, self.inner.as_ref(), &self.spans)
+    }
+}
+
+pub trait Diagnostic {
+    fn diagnostic(&self) -> codespan_reporting::diagnostic::Diagnostic<()>;
 
     /// Emits a summary of the error to standard error stream.
-    pub fn emit_to_stderr(&self, source: &str)
-    where
-        E: Error,
-    {
+    fn emit_to_stderr(&self, source: &str) {
         self.emit_to_stderr_with_path(source, "wgsl")
     }
 
     /// Emits a summary of the error to standard error stream.
-    pub fn emit_to_stderr_with_path(&self, source: &str, path: &str)
-    where
-        E: Error,
-    {
+    fn emit_to_stderr_with_path(&self, source: &str, path: &str) {
         use codespan_reporting::term::termcolor::{ColorChoice, StandardStream};
         use codespan_reporting::{files, term};
 
@@ -296,18 +337,12 @@ impl<E> WithSpan<E> {
     }
 
     /// Emits a summary of the error to a string.
-    pub fn emit_to_string(&self, source: &str) -> String
-    where
-        E: Error,
-    {
+    fn emit_to_string(&self, source: &str) -> String {
         self.emit_to_string_with_path(source, "wgsl")
     }
 
     /// Emits a summary of the error to a string.
-    pub fn emit_to_string_with_path(&self, source: &str, path: &str) -> String
-    where
-        E: Error,
-    {
+    fn emit_to_string_with_path(&self, source: &str, path: &str) -> String {
         use codespan_reporting::term::termcolor::NoColor;
         use codespan_reporting::{files, term};
 

naga/src/valid/function.rs

@@ -7,6 +7,7 @@ use super::{
 };
 use crate::arena::{Arena, UniqueArena};
 use crate::arena::{Handle, HandleSet};
+use crate::diagnostic_filter::Severity;
 use crate::proc::TypeResolution;
 use crate::span::WithSpan;
 use crate::span::{AddSpan as _, MapErrWithSpan as _};
@@ -112,8 +113,8 @@ pub enum FunctionError {
         name: String,
         space: crate::AddressSpace,
     },
-    #[error("There are instructions after `return`/`break`/`continue`")]
-    InstructionsAfterReturn,
+    #[error("Unreachable statement after `{after}`")]
+    UnreachableStatement { after: &'static str },
     #[error("The `break` is used outside of a `loop` or `switch` context")]
     BreakOutsideOfLoopOrSwitch,
     #[error("The `continue` is used outside of a `loop` context")]
@@ -232,9 +233,18 @@ bitflags::bitflags! {
     }
 }
 
+/// Return type of `validate_block`
 struct BlockInfo {
+    /// Shader stages for which the block is valid.
     stages: super::ShaderStages,
-    finished: bool,
+
+    /// Span for a control flow statement that terminated the block.
+    ///
+    /// If the block contained a statement like `return`, `break`, `continue`,
+    /// or `discard` that will cause further statements not to be executed, this
+    /// is set based on that statement. This is used for "unreachable statement"
+    /// diagnostics.
+    unreachable: Option<(&'static str, crate::Span)>,
 }
 
 struct BlockContext<'a> {
@@ -751,12 +761,29 @@ impl super::Validator {
         context: &BlockContext,
     ) -> Result<BlockInfo, WithSpan<FunctionError>> {
         use crate::{AddressSpace, Statement as S, TypeInner as Ti};
-        let mut finished = false;
+        // Information used for "unreachable statement" diagnostics. Upon
+        // diverging control flow like `return`, this is set to `Some(Some(_))`.
+        // After emitting a diagnostic, this is set to `Some(None)`, so that
+        // there is only one warning for each block containing unreachable
+        // statements.
+        let mut unreachable = None;
         let mut stages = super::ShaderStages::all();
         for (statement, &span) in statements.span_iter() {
-            if finished {
-                return Err(FunctionError::InstructionsAfterReturn
-                    .with_span_static(span, "instructions after return"));
+            if let Some(Some((prev_stmt, prev_span))) = unreachable {
+                Severity::Warning.report_diag(
+                    FunctionError::UnreachableStatement { after: prev_stmt }
+                        .with_span()
+                        .with_context((span, String::from("this statement is unreachable")))
+                        .with_context((
+                            prev_span,
+                            String::from("because it appears after this statement"),
+                        )),
+                    |e, level| {
+                        log::log!(level, "{e}");
+                        self.diagnostics.push(e.boxed());
+                    },
+                )?;
+                unreachable = Some(None);
             }
             match *statement {
                 S::Emit(ref range) => {
@@ -806,7 +833,7 @@ impl super::Validator {
                 S::Block(ref block) => {
                     let info = self.validate_block(block, context)?;
                     stages &= info.stages;
-                    finished = info.finished;
+                    unreachable.get_or_insert(info.unreachable);
                 }
                 S::If {
                     condition,
@@ -949,14 +976,14 @@ impl super::Validator {
                         return Err(FunctionError::BreakOutsideOfLoopOrSwitch
                             .with_span_static(span, "invalid break"));
                     }
-                    finished = true;
+                    unreachable.get_or_insert(Some(("break", span)));
                 }
                 S::Continue => {
                     if !context.abilities.contains(ControlFlowAbility::CONTINUE) {
                         return Err(FunctionError::ContinueOutsideOfLoop
                             .with_span_static(span, "invalid continue"));
                     }
-                    finished = true;
+                    unreachable.get_or_insert(Some(("continue", span)));
                 }
                 S::Return { value } => {
                     if !context.abilities.contains(ControlFlowAbility::RETURN) {
@@ -996,11 +1023,11 @@ impl super::Validator {
                             .with_span_static(span, "invalid return"));
                         }
                     }
-                    finished = true;
+                    unreachable.get_or_insert(Some(("return", span)));
                 }
                 S::Kill => {
                     stages &= super::ShaderStages::FRAGMENT;
-                    finished = true;
+                    unreachable.get_or_insert(Some(("discard", span)));
                 }
                 S::Barrier(barrier) => {
                     stages &= super::ShaderStages::COMPUTE;
@@ -1618,7 +1645,10 @@ impl super::Validator {
                 }
             }
         }
-        Ok(BlockInfo { stages, finished })
+        Ok(BlockInfo {
+            stages,
+            unreachable: unreachable.unwrap_or(None),
+        })
     }
 
     fn validate_block(

naga/src/valid/mod.rs

@@ -18,6 +18,7 @@ use bit_set::BitSet;
 use crate::{
     arena::{Handle, HandleSet},
     proc::{ExpressionKindTracker, LayoutError, Layouter, TypeResolution},
+    span::WithSpanBoxed,
     FastHashSet,
 };
 
@@ -283,6 +284,7 @@ pub struct Validator {
     switch_values: FastHashSet<crate::SwitchValue>,
     valid_expression_list: Vec<Handle<crate::Expression>>,
     valid_expression_set: HandleSet<crate::Expression>,
+    diagnostics: Vec<WithSpanBoxed>,
     override_ids: FastHashSet<u16>,
 
     /// Treat overrides whose initializers are not fully-evaluated
@@ -485,6 +487,7 @@ impl Validator {
             switch_values: FastHashSet::default(),
             valid_expression_list: Vec::new(),
             valid_expression_set: HandleSet::new(),
+            diagnostics: Vec::new(),
             override_ids: FastHashSet::default(),
             overrides_resolved: false,
             needs_visit: HandleSet::new(),
@@ -511,6 +514,7 @@ impl Validator {
         self.switch_values.clear();
         self.valid_expression_list.clear();
         self.valid_expression_set.clear();
+        self.diagnostics.clear();
         self.override_ids.clear();
     }
 
@@ -758,6 +762,13 @@ impl Validator {
 
         Ok(mod_info)
     }
+
+    /// Return any warnings produced during validation.
+    ///
+    /// The details of returned diagnostic messages are not a stable API.
+    pub fn diagnostics(&self) -> &[WithSpanBoxed] {
+        &self.diagnostics
+    }
 }
 
 fn validate_atomic_compare_exchange_struct(