Skip to content

Commit

Permalink
Add support for 'or' expressions
Browse files Browse the repository at this point in the history
This particular kind of expression is tricky to implement because of a
special case of backwards compatibility with a rarely used Nixpkgs
function also named `or` (see [parser.y] for more details).

[parser.y]: https://github.com/NixOS/nix/blob/f8b30338ac231262bdf19844f044f0572c460048/src/libexpr/parser.y#L376-L378

Implementing support for this fallback case incurs a slight performance
penalty, as shown in the Criterion benchmark results below:

```
parse example.nix       time:   [681.43 us 683.99 us 686.99 us]
                        change: [-0.9655% +2.6817% +6.4750%] (p = 0.16 > 0.05)
                        No change in performance detected.
Found 13 outliers among 100 measurements (13.00%)
  5 (5.00%) high mild
  8 (8.00%) high severe
```

Also, since the `or` keyword is only ever used in projections and never
on its own, we can safely remove the `ExprOr` type and rely entirely on
the `fallback` argument for `ExprProj::new()` instead.
  • Loading branch information
ebkalderon committed Oct 13, 2019
1 parent a1e4c50 commit 82149a5
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 64 deletions.
55 changes: 1 addition & 54 deletions nix-parser/src/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,13 +78,11 @@ pub enum Expr {
Let(ExprLet),
/// `rec { foo = "bar"; }`
Rec(ExprRec),
/// `x.y`
/// `x.y`, `x.y or "true"`
Proj(Box<ExprProj>),

/// `if true then "success" else "failure"`
If(Box<ExprIf>),
/// `foo.bar or "failed"`
Or(Box<ExprOr>),
/// `assert true != false; true`
Assert(Box<ExprAssert>),
/// `with foo; foo.attr`
Expand Down Expand Up @@ -122,7 +120,6 @@ impl Display for Expr {
Expr::Proj(ref e) => write!(fmt, "{}", e),

Expr::If(ref e) => write!(fmt, "{}", e),
Expr::Or(ref e) => write!(fmt, "{}", e),
Expr::Assert(ref e) => write!(fmt, "{}", e),
Expr::With(ref e) => write!(fmt, "{}", e),

Expand Down Expand Up @@ -167,7 +164,6 @@ impl HasSpan for Expr {
Expr::Proj(ref e) => e.span(),

Expr::If(ref e) => e.span(),
Expr::Or(ref e) => e.span(),
Expr::Assert(ref e) => e.span(),
Expr::With(ref e) => e.span(),

Expand Down Expand Up @@ -1090,55 +1086,6 @@ impl PartialEq for ExprIf {
}
}

#[derive(Clone, Debug)]
pub struct ExprOr {
expr: Expr,
fallback: Expr,
span: Span,
}

impl ExprOr {
pub fn new(expr: Expr, fallback: Expr, span: Span) -> Self {
ExprOr {
expr,
fallback,
span,
}
}

pub fn expr(&self) -> &Expr {
&self.expr
}

pub fn fallback(&self) -> &Expr {
&self.fallback
}
}

impl Display for ExprOr {
fn fmt(&self, fmt: &mut Formatter) -> FmtResult {
write!(fmt, "{} or {}", self.expr, self.fallback)
}
}

impl HasSpan for ExprOr {
fn span(&self) -> Span {
self.span
}
}

impl From<ExprOr> for Expr {
fn from(e: ExprOr) -> Expr {
Expr::Or(Box::new(e))
}
}

impl PartialEq for ExprOr {
fn eq(&self, other: &Self) -> bool {
self.expr == other.expr && self.fallback == other.fallback
}
}

#[derive(Clone, Debug)]
pub struct ExprAssert {
cond: Expr,
Expand Down
41 changes: 31 additions & 10 deletions nix-parser/src/parser/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,10 @@ use nom::multi::many0;
use nom::sequence::{pair, preceded};

use super::partial::{
expect_terminated, map_partial, map_partial_spanned, pair_partial, verify_full, Partial,
expect_terminated, map_partial, map_partial_spanned, opt_partial, pair_partial, Partial,
};
use super::{tokens, IResult};
use crate::ast::tokens::Ident;
use crate::ast::{BinaryOp, Expr, ExprBinary, ExprFnApp, ExprIf, ExprProj, ExprUnary, UnaryOp};
use crate::error::{Errors, UnexpectedError};
use crate::lexer::{Token, Tokens};
Expand Down Expand Up @@ -213,15 +214,35 @@ fn fn_app(input: Tokens) -> IResult<Partial<Expr>> {
}

fn project(input: Tokens) -> IResult<Partial<Expr>> {
let path = preceded(tokens::dot, verify_full(attr::attr_path));
let expr = pair(atomic, opt(path));
map(expr, |(base, path)| match path {
None => base,
Some(path) => base.map(|base| {
let span = Span::merge(base.span(), path.span());
Expr::Proj(Box::new(ExprProj::new(base, path, None, span)))
}),
})(input)
let (input, atomic) = atomic(input)?;

if let Ok((remaining, _)) = tokens::dot(input) {
let default = alt((project, error, util::error_expr_if(tokens::eof, "<eof>")));
let or_default = opt_partial(preceded(tokens::keyword_or, default));

let (remaining, path) = pair_partial(attr::attr_path, or_default)(remaining)?;
let proj = atomic.flat_map(|atomic| {
path.map(|(path, default)| {
let span = Span::merge(atomic.span(), path.span());
match default {
Some(expr) => Expr::from(ExprProj::new(atomic, path, Some(expr), span)),
None => Expr::from(ExprProj::new(atomic, path, None, span)),
}
})
});

Ok((remaining, proj))
} else if let Ok((remaining, or_span)) = tokens::keyword_or(input) {
let expr = atomic.map(|atomic| {
let arg = Expr::Ident(Ident::from(("or", or_span)));
let span = Span::merge(atomic.span(), or_span);
Expr::from(ExprFnApp::new(atomic, arg, span))
});

Ok((remaining, expr))
} else {
Ok((input, atomic))
}
}

fn atomic(input: Tokens) -> IResult<Partial<Expr>> {
Expand Down

0 comments on commit 82149a5

Please sign in to comment.