Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make valid::expressions module depend on "validate" feature. #2480

Closed
wants to merge 1 commit into from
Closed

Make valid::expressions module depend on "validate" feature. #2480

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
131 changes: 3 additions & 128 deletions src/valid/expression.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,139 +1,15 @@
#[cfg(feature = "validate")]
use super::{
compose::validate_compose, validate_atomic_compare_exchange_struct, FunctionInfo, ModuleInfo,
ShaderStages, TypeFlags,
compose::validate_compose, validate_atomic_compare_exchange_struct, ExpressionError,
FunctionInfo, ModuleInfo, ShaderStages, TypeFlags,
};
#[cfg(feature = "validate")]
use crate::arena::UniqueArena;
use crate::arena::{Handle, UniqueArena};

use crate::{
arena::Handle,
proc::{IndexableLengthError, ResolveError},
};

#[derive(Clone, Debug, thiserror::Error)]
#[cfg_attr(test, derive(PartialEq))]
pub enum ExpressionError {
#[error("Doesn't exist")]
DoesntExist,
#[error("Used by a statement before it was introduced into the scope by any of the dominating blocks")]
NotInScope,
#[error("Base type {0:?} is not compatible with this expression")]
InvalidBaseType(Handle<crate::Expression>),
#[error("Accessing with index {0:?} can't be done")]
InvalidIndexType(Handle<crate::Expression>),
#[error("Accessing {0:?} via a negative index is invalid")]
NegativeIndex(Handle<crate::Expression>),
#[error("Accessing index {1} is out of {0:?} bounds")]
IndexOutOfBounds(Handle<crate::Expression>, u32),
#[error("The expression {0:?} may only be indexed by a constant")]
IndexMustBeConstant(Handle<crate::Expression>),
#[error("Function argument {0:?} doesn't exist")]
FunctionArgumentDoesntExist(u32),
#[error("Loading of {0:?} can't be done")]
InvalidPointerType(Handle<crate::Expression>),
#[error("Array length of {0:?} can't be done")]
InvalidArrayType(Handle<crate::Expression>),
#[error("Get intersection of {0:?} can't be done")]
InvalidRayQueryType(Handle<crate::Expression>),
#[error("Splatting {0:?} can't be done")]
InvalidSplatType(Handle<crate::Expression>),
#[error("Swizzling {0:?} can't be done")]
InvalidVectorType(Handle<crate::Expression>),
#[error("Swizzle component {0:?} is outside of vector size {1:?}")]
InvalidSwizzleComponent(crate::SwizzleComponent, crate::VectorSize),
#[error(transparent)]
Compose(#[from] super::ComposeError),
#[error(transparent)]
IndexableLength(#[from] IndexableLengthError),
#[error("Operation {0:?} can't work with {1:?}")]
InvalidUnaryOperandType(crate::UnaryOperator, Handle<crate::Expression>),
#[error("Operation {0:?} can't work with {1:?} and {2:?}")]
InvalidBinaryOperandTypes(
crate::BinaryOperator,
Handle<crate::Expression>,
Handle<crate::Expression>,
),
#[error("Selecting is not possible")]
InvalidSelectTypes,
#[error("Relational argument {0:?} is not a boolean vector")]
InvalidBooleanVector(Handle<crate::Expression>),
#[error("Relational argument {0:?} is not a float")]
InvalidFloatArgument(Handle<crate::Expression>),
#[error("Type resolution failed")]
Type(#[from] ResolveError),
#[error("Not a global variable")]
ExpectedGlobalVariable,
#[error("Not a global variable or a function argument")]
ExpectedGlobalOrArgument,
#[error("Needs to be an binding array instead of {0:?}")]
ExpectedBindingArrayType(Handle<crate::Type>),
#[error("Needs to be an image instead of {0:?}")]
ExpectedImageType(Handle<crate::Type>),
#[error("Needs to be an image instead of {0:?}")]
ExpectedSamplerType(Handle<crate::Type>),
#[error("Unable to operate on image class {0:?}")]
InvalidImageClass(crate::ImageClass),
#[error("Derivatives can only be taken from scalar and vector floats")]
InvalidDerivative,
#[error("Image array index parameter is misplaced")]
InvalidImageArrayIndex,
#[error("Inappropriate sample or level-of-detail index for texel access")]
InvalidImageOtherIndex,
#[error("Image array index type of {0:?} is not an integer scalar")]
InvalidImageArrayIndexType(Handle<crate::Expression>),
#[error("Image sample or level-of-detail index's type of {0:?} is not an integer scalar")]
InvalidImageOtherIndexType(Handle<crate::Expression>),
#[error("Image coordinate type of {1:?} does not match dimension {0:?}")]
InvalidImageCoordinateType(crate::ImageDimension, Handle<crate::Expression>),
#[error("Comparison sampling mismatch: image has class {image:?}, but the sampler is comparison={sampler}, and the reference was provided={has_ref}")]
ComparisonSamplingMismatch {
image: crate::ImageClass,
sampler: bool,
has_ref: bool,
},
#[error("Sample offset constant {1:?} doesn't match the image dimension {0:?}")]
InvalidSampleOffset(crate::ImageDimension, Handle<crate::Expression>),
#[error("Depth reference {0:?} is not a scalar float")]
InvalidDepthReference(Handle<crate::Expression>),
#[error("Depth sample level can only be Auto or Zero")]
InvalidDepthSampleLevel,
#[error("Gather level can only be Zero")]
InvalidGatherLevel,
#[error("Gather component {0:?} doesn't exist in the image")]
InvalidGatherComponent(crate::SwizzleComponent),
#[error("Gather can't be done for image dimension {0:?}")]
InvalidGatherDimension(crate::ImageDimension),
#[error("Sample level (exact) type {0:?} is not a scalar float")]
InvalidSampleLevelExactType(Handle<crate::Expression>),
#[error("Sample level (bias) type {0:?} is not a scalar float")]
InvalidSampleLevelBiasType(Handle<crate::Expression>),
#[error("Sample level (gradient) of {1:?} doesn't match the image dimension {0:?}")]
InvalidSampleLevelGradientType(crate::ImageDimension, Handle<crate::Expression>),
#[error("Unable to cast")]
InvalidCastArgument,
#[error("Invalid argument count for {0:?}")]
WrongArgumentCount(crate::MathFunction),
#[error("Argument [{1}] to {0:?} as expression {2:?} has an invalid type.")]
InvalidArgumentType(crate::MathFunction, u32, Handle<crate::Expression>),
#[error("Atomic result type can't be {0:?}")]
InvalidAtomicResultType(Handle<crate::Type>),
#[error(
"workgroupUniformLoad result type can't be {0:?}. It can only be a constructible type."
)]
InvalidWorkGroupUniformLoadResultType(Handle<crate::Type>),
#[error("Shader requires capability {0:?}")]
MissingCapabilities(super::Capabilities),
}

#[cfg(feature = "validate")]
struct ExpressionTypeResolver<'a> {
root: Handle<crate::Expression>,
types: &'a UniqueArena<crate::Type>,
info: &'a FunctionInfo,
}

#[cfg(feature = "validate")]
impl<'a> std::ops::Index<Handle<crate::Expression>> for ExpressionTypeResolver<'a> {
type Output = crate::TypeInner;

Expand All @@ -151,7 +27,6 @@ impl<'a> std::ops::Index<Handle<crate::Expression>> for ExpressionTypeResolver<'
}
}

#[cfg(feature = "validate")]
impl super::Validator {
pub(super) fn validate_const_expression(
&self,
Expand Down
121 changes: 118 additions & 3 deletions src/valid/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ Shader validator.

mod analyzer;
mod compose;
#[cfg(feature = "validate")]
mod expression;
mod function;
mod handles;
Expand All @@ -12,7 +13,7 @@ mod r#type;

use crate::{
arena::Handle,
proc::{LayoutError, Layouter, TypeResolution},
proc::{IndexableLengthError, LayoutError, Layouter, ResolveError, TypeResolution},
FastHashSet,
};
use bit_set::BitSet;
Expand All @@ -24,7 +25,6 @@ use std::ops;
use crate::span::{AddSpan as _, WithSpan};
pub use analyzer::{ExpressionInfo, FunctionInfo, GlobalUse, Uniformity, UniformityRequirements};
pub use compose::ComposeError;
pub use expression::ExpressionError;
pub use function::{CallError, FunctionError, LocalVariableError};
pub use interface::{EntryPointError, GlobalVariableError, VaryingError};
pub use r#type::{Disalignment, TypeError, TypeFlags};
Expand Down Expand Up @@ -187,7 +187,7 @@ pub enum ConstExpressionError {
#[error(transparent)]
Compose(#[from] ComposeError),
#[error("Type resolution failed")]
Type(#[from] crate::proc::ResolveError),
Type(#[from] ResolveError),
}

#[derive(Clone, Debug, thiserror::Error)]
Expand Down Expand Up @@ -243,6 +243,121 @@ pub enum ValidationError {
Corrupted,
}

#[derive(Clone, Debug, thiserror::Error)]
#[cfg_attr(test, derive(PartialEq))]
pub enum ExpressionError {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have to move this out? Can we make it conditional on the feature flag instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with making it conditional is that that changes the API of the validator. We want turning off "validate" to simply make producing ModuleInfo faster, without changing its API.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, clients that happen to be checking for particular ExpressionError variants shouldn't break.

Hmm. Maybe it's preferable for any users checking for FunctionError::Expression to get a compilation error if the "validation" feature is off, since that error will never be generated and users shouldn't be thinking it could happen?

API stability and deliberate API instability both seem to have arguments in their favor. Which do you think is more useful?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's natural for feature flags to expose more of the API.

For users that turn on the flag, all enum variants of ValidationError will be present, for those who don't they may or may not be there but I don't think those users would exhaustively match the enum anyway.

#[error("Doesn't exist")]
DoesntExist,
#[error("Used by a statement before it was introduced into the scope by any of the dominating blocks")]
NotInScope,
#[error("Base type {0:?} is not compatible with this expression")]
InvalidBaseType(Handle<crate::Expression>),
#[error("Accessing with index {0:?} can't be done")]
InvalidIndexType(Handle<crate::Expression>),
#[error("Accessing {0:?} via a negative index is invalid")]
NegativeIndex(Handle<crate::Expression>),
#[error("Accessing index {1} is out of {0:?} bounds")]
IndexOutOfBounds(Handle<crate::Expression>, u32),
#[error("The expression {0:?} may only be indexed by a constant")]
IndexMustBeConstant(Handle<crate::Expression>),
#[error("Function argument {0:?} doesn't exist")]
FunctionArgumentDoesntExist(u32),
#[error("Loading of {0:?} can't be done")]
InvalidPointerType(Handle<crate::Expression>),
#[error("Array length of {0:?} can't be done")]
InvalidArrayType(Handle<crate::Expression>),
#[error("Get intersection of {0:?} can't be done")]
InvalidRayQueryType(Handle<crate::Expression>),
#[error("Splatting {0:?} can't be done")]
InvalidSplatType(Handle<crate::Expression>),
#[error("Swizzling {0:?} can't be done")]
InvalidVectorType(Handle<crate::Expression>),
#[error("Swizzle component {0:?} is outside of vector size {1:?}")]
InvalidSwizzleComponent(crate::SwizzleComponent, crate::VectorSize),
#[error(transparent)]
Compose(#[from] ComposeError),
#[error(transparent)]
IndexableLength(#[from] IndexableLengthError),
#[error("Operation {0:?} can't work with {1:?}")]
InvalidUnaryOperandType(crate::UnaryOperator, Handle<crate::Expression>),
#[error("Operation {0:?} can't work with {1:?} and {2:?}")]
InvalidBinaryOperandTypes(
crate::BinaryOperator,
Handle<crate::Expression>,
Handle<crate::Expression>,
),
#[error("Selecting is not possible")]
InvalidSelectTypes,
#[error("Relational argument {0:?} is not a boolean vector")]
InvalidBooleanVector(Handle<crate::Expression>),
#[error("Relational argument {0:?} is not a float")]
InvalidFloatArgument(Handle<crate::Expression>),
#[error("Type resolution failed")]
Type(#[from] ResolveError),
#[error("Not a global variable")]
ExpectedGlobalVariable,
#[error("Not a global variable or a function argument")]
ExpectedGlobalOrArgument,
#[error("Needs to be an binding array instead of {0:?}")]
ExpectedBindingArrayType(Handle<crate::Type>),
#[error("Needs to be an image instead of {0:?}")]
ExpectedImageType(Handle<crate::Type>),
#[error("Needs to be an image instead of {0:?}")]
ExpectedSamplerType(Handle<crate::Type>),
#[error("Unable to operate on image class {0:?}")]
InvalidImageClass(crate::ImageClass),
#[error("Derivatives can only be taken from scalar and vector floats")]
InvalidDerivative,
#[error("Image array index parameter is misplaced")]
InvalidImageArrayIndex,
#[error("Inappropriate sample or level-of-detail index for texel access")]
InvalidImageOtherIndex,
#[error("Image array index type of {0:?} is not an integer scalar")]
InvalidImageArrayIndexType(Handle<crate::Expression>),
#[error("Image sample or level-of-detail index's type of {0:?} is not an integer scalar")]
InvalidImageOtherIndexType(Handle<crate::Expression>),
#[error("Image coordinate type of {1:?} does not match dimension {0:?}")]
InvalidImageCoordinateType(crate::ImageDimension, Handle<crate::Expression>),
#[error("Comparison sampling mismatch: image has class {image:?}, but the sampler is comparison={sampler}, and the reference was provided={has_ref}")]
ComparisonSamplingMismatch {
image: crate::ImageClass,
sampler: bool,
has_ref: bool,
},
#[error("Sample offset constant {1:?} doesn't match the image dimension {0:?}")]
InvalidSampleOffset(crate::ImageDimension, Handle<crate::Expression>),
#[error("Depth reference {0:?} is not a scalar float")]
InvalidDepthReference(Handle<crate::Expression>),
#[error("Depth sample level can only be Auto or Zero")]
InvalidDepthSampleLevel,
#[error("Gather level can only be Zero")]
InvalidGatherLevel,
#[error("Gather component {0:?} doesn't exist in the image")]
InvalidGatherComponent(crate::SwizzleComponent),
#[error("Gather can't be done for image dimension {0:?}")]
InvalidGatherDimension(crate::ImageDimension),
#[error("Sample level (exact) type {0:?} is not a scalar float")]
InvalidSampleLevelExactType(Handle<crate::Expression>),
#[error("Sample level (bias) type {0:?} is not a scalar float")]
InvalidSampleLevelBiasType(Handle<crate::Expression>),
#[error("Sample level (gradient) of {1:?} doesn't match the image dimension {0:?}")]
InvalidSampleLevelGradientType(crate::ImageDimension, Handle<crate::Expression>),
#[error("Unable to cast")]
InvalidCastArgument,
#[error("Invalid argument count for {0:?}")]
WrongArgumentCount(crate::MathFunction),
#[error("Argument [{1}] to {0:?} as expression {2:?} has an invalid type.")]
InvalidArgumentType(crate::MathFunction, u32, Handle<crate::Expression>),
#[error("Atomic result type can't be {0:?}")]
InvalidAtomicResultType(Handle<crate::Type>),
#[error(
"workgroupUniformLoad result type can't be {0:?}. It can only be a constructible type."
)]
InvalidWorkGroupUniformLoadResultType(Handle<crate::Type>),
#[error("Shader requires capability {0:?}")]
MissingCapabilities(Capabilities),
}

impl crate::TypeInner {
#[cfg(feature = "validate")]
const fn is_sized(&self) -> bool {
Expand Down