Skip to content

Commit

Permalink
chore(deps): drop dependency on lexical
Browse files Browse the repository at this point in the history
The `lexical` crate maintenance status has been called into question
while being affected by several soundness issues, as explained in
[`RUSTSEC-2023-0055`](https://rustsec.org/advisories/RUSTSEC-2023-0055).
However, as far as I can see, we don't really need to use `lexical` in
the GLSL preprocessor:

- For parsing `#version` numbers, `lexical` didn't provide results that
  complied with my interpretation of the GLSL specification, which I
  explained in a code comment, and that I believe motivated a TODO item
  to move on from `lexical`. Moreover, as `#version` directives are
  expected to be infrequent (usually, at most one per translation unit),
  I don't think it's worth to bloat our dependencies with
  `atoi(_simd|_radix10)`, as `#version` numbers are tiny 16-bit integers
  where SIMD and bitwise optimizations have a neglible performance
  impact, so these changes go for correctness and use the INT and UINT
  token parsing routines to parse these.
- For parsing floating point numbers, modern Rust stdlib versions ship
  a performant algorithm proposed by `lexical`'s author, which negate
  previous performance gaps between both implementations. Therefore,
  just use the standard parsing functions in these cases.

As far as I am aware, these changes are not semver-breaking because the
modified `VersionError` enum was never exposed outside of the `lang-pp`
crate.
  • Loading branch information
AlexTMjugador committed Oct 22, 2023
1 parent d73718f commit 5dc7eb2
Show file tree
Hide file tree
Showing 4 changed files with 43 additions and 15 deletions.
1 change: 0 additions & 1 deletion lang-pp/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ lang-util = "0.5.0"

string_cache = "0.8"
thiserror = "1.0"
lexical = "6.1"
arrayvec = "0.7"
derive_more = "0.99"

Expand Down
9 changes: 7 additions & 2 deletions lang-pp/src/processor/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use std::iter::Peekable;

use crate::{
parser::SyntaxKind::{self, *},
types::Token,
util::Unescaped,
};

Expand Down Expand Up @@ -66,8 +67,12 @@ impl<'i, I: Iterator<Item = &'i OutputToken>> ExprEvaluator<'i, I> {
DIGITS => {
// Try to parse the value before bumping. If parsing fails, we'll return the DIGITS
// token unparsed
if let Ok(value) = lexical::parse(Unescaped::new(token.text()).to_string().as_ref())
{
let value = match Token::parse_digits(&Unescaped::new(token.text()).to_string()) {
Token::UINT_CONST(value) if value <= i32::MAX as u32 => Some(value as i32),
Token::INT_CONST(value) => Some(value),
_ => None,
};
if let Some(value) = value {
self.bump();
return Some(Ok(value));
}
Expand Down
35 changes: 30 additions & 5 deletions lang-pp/src/processor/nodes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@ use crate::{
exts::names::ExtNameAtom,
parser::{SyntaxKind::*, SyntaxNode, SyntaxToken},
processor::expr::{EvalResult, ExprEvaluator},
types::path::{ParsedPath, PathType},
types::{
path::{ParsedPath, PathType},
Token,
},
util::Unescaped,
};

Expand Down Expand Up @@ -123,7 +126,7 @@ pub enum VersionError {
#[error("missing version number in #version directive")]
MissingVersionNumber,
#[error("invalid version number in #version directive")]
InvalidVersionNumber(#[from] lexical::Error),
InvalidVersionNumber { version_number: String },
#[error("unsupported version number in #version directive")]
UnsupportedVersionNumber,
#[error("invalid version profile")]
Expand All @@ -143,7 +146,17 @@ impl TryFrom<(FileId, SyntaxNode)> for Version {

fn try_from((_file_id, value): (FileId, SyntaxNode)) -> Result<Self, Self::Error> {
// Parse version number
// TODO: Replace use of lexical with glsl parser
// The GLSL spec refers to the ISO/IEC 14882:1998 (C++) standard for preprocessor
// directives, and mentions that #version, due to it "following the same convention"
// as __VERSION__, is "a decimal integer". The cited standard, section 2.9
// "Preprocessing numbers", says that such numbers "lexically include all integral
// literal tokens". These correspond to the "integer-constant" (for the GLSL grammar)
// or "integer-literal" (for the C++ grammar) grammar symbols. But, because #version
// is a decimal integer, it is understood that the only "integer-constant" production
// rule we have to accept is the one for decimal constants, which are sequences of
// decimal digits beginning with a nonzero digit and followed by an optional
// "integer-suffix" (u or U character). This is a subset of what we can parse as
// (U)INT_CONST tokens, but other implementations are likely to observe Postel's law
let version_number: u16 = value
.children()
.find_map(|token| {
Expand All @@ -155,8 +168,20 @@ impl TryFrom<(FileId, SyntaxNode)> for Version {
})
.ok_or(Self::Error::MissingVersionNumber)
.and_then(|token| {
lexical::parse(Unescaped::new(token.text()).to_string().as_ref())
.map_err(Into::into)
let token_string = Unescaped::new(token.text()).to_string();
match Token::parse_digits(&token_string) {
Token::INT_CONST(version_number)
if version_number >= 0 && version_number <= u16::MAX as i32 =>
{
Ok(version_number as u16)
}
Token::UINT_CONST(version_number) if version_number <= u16::MAX as u32 => {
Ok(version_number as u16)
}
_ => Err(Self::Error::InvalidVersionNumber {
version_number: token_string.into_owned(),
}),
}
})?;

// Check the version number is supported
Expand Down
13 changes: 6 additions & 7 deletions lang-pp/src/types/token.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1983,7 +1983,7 @@ impl Token {
}

fn strip_suffix(text: &str) -> (bool, &str) {
if let Some(stripped) = text.strip_suffix('u').or_else(|| text.strip_suffix('U')) {
if let Some(stripped) = text.strip_suffix(&['u', 'U']) {
(true, stripped)
} else {
(false, text)
Expand Down Expand Up @@ -2025,14 +2025,13 @@ impl Token {
// Floating-point constant

if let Some(double) = text.strip_suffix("lf").or_else(|| text.strip_suffix("LF")) {
lexical::parse(double)
double
.parse()
.map(DOUBLE_CONST)
.map_err(|_| ErrorKind::InvalidDoubleLiteral)
} else if let Some(float) = text
.strip_suffix('f')
.or_else(|| text.strip_suffix('F').or(Some(text)))
{
lexical::parse(float)
} else if let Some(float) = text.strip_suffix(&['f', 'F']).or(Some(text)) {
float
.parse()
.map(FLOAT_CONST)
.map_err(|_| ErrorKind::InvalidFloatLiteral)
} else {
Expand Down

0 comments on commit 5dc7eb2

Please sign in to comment.