Skip to content

Commit

Permalink
[internal] Parse addresses using a generated parser (#14346)
Browse files Browse the repository at this point in the history
To prepare to make the address syntax more complicated as part of #13882, move to parsing addresses using a generated `peg` parser (using the same parser crate that @jsirois introduced in #11922).

Additionally, simplify `Address.spec` and `Address.path_safe_spec` slightly by removing non-trivial early returns and reducing the total number of f-strings.

[ci skip-build-wheels]
  • Loading branch information
stuhood authored Feb 3, 2022
1 parent e535ce2 commit 397e685
Show file tree
Hide file tree
Showing 10 changed files with 177 additions and 35 deletions.
60 changes: 29 additions & 31 deletions src/python/pants/build_graph/address.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@
from typing import Any, Sequence

from pants.engine.engine_aware import EngineAwareParameter
from pants.engine.internals import native_engine
from pants.engine.internals.native_engine import ( # noqa: F401
AddressParseException as AddressParseException,
)
from pants.util.dirutil import fast_relpath, longest_dir_prefix
from pants.util.strutil import strip_prefix

Expand Down Expand Up @@ -127,22 +131,7 @@ def prefix_subproject(spec_path: str) -> str:
return os.path.join(subproject, spec_path)
return os.path.normpath(subproject)

spec_parts = spec.split(":", maxsplit=1)
path_component = spec_parts[0]
if len(spec_parts) == 1:
target_component = None
generated_parts = path_component.split("#", maxsplit=1)
if len(generated_parts) == 1:
generated_component = None
else:
path_component, generated_component = generated_parts
else:
generated_parts = spec_parts[1].split("#", maxsplit=1)
if len(generated_parts) == 1:
target_component = generated_parts[0]
generated_component = None
else:
target_component, generated_component = generated_parts
path_component, target_component, generated_component = native_engine.address_parse(spec)

normalized_relative_to = None
if relative_to:
Expand Down Expand Up @@ -324,18 +313,26 @@ def spec(self) -> str:
:API: public
"""
prefix = "//" if not self.spec_path else ""
if self._relative_file_path is not None:
file_portion = f"{prefix}{self.filename}"
if self._relative_file_path is None:
path = self.spec_path
target = "" if self._target_name is None and self.generated_name else self.target_name
else:
path = self.filename
parent_prefix = "../" * self._relative_file_path.count(os.path.sep)
return (
file_portion
target = (
""
if self._target_name is None and not parent_prefix
else f"{file_portion}:{parent_prefix}{self.target_name}"
else f"{parent_prefix}{self.target_name}"
)
if self.generated_name is not None:
target_portion = f":{self._target_name}" if self._target_name is not None else ""
return f"{prefix}{self.spec_path}{target_portion}#{self.generated_name}"
return f"{prefix}{self.spec_path}:{self.target_name}"
target_sep = ":" if target else ""
if self.generated_name is None:
generated_sep = ""
generated = ""
else:
generated_sep = "#"
generated = self.generated_name

return f"{prefix}{path}{target_sep}{target}{generated_sep}{generated}"

@property
def path_safe_spec(self) -> str:
Expand All @@ -345,18 +342,19 @@ def path_safe_spec(self) -> str:
if self._relative_file_path:
parent_count = self._relative_file_path.count(os.path.sep)
parent_prefix = "@" * parent_count if parent_count else "."
file_portion = f".{self._relative_file_path.replace(os.path.sep, '.')}"
path = f".{self._relative_file_path.replace(os.path.sep, '.')}"
else:
parent_prefix = "."
file_portion = ""
path = ""
if parent_prefix == ".":
target_portion = f"{parent_prefix}{self._target_name}" if self._target_name else ""
target = f"{parent_prefix}{self._target_name}" if self._target_name else ""
else:
target_portion = f"{parent_prefix}{self.target_name}"
generated_portion = (
target = f"{parent_prefix}{self.target_name}"
generated = (
f"@{self.generated_name.replace(os.path.sep, '.')}" if self.generated_name else ""
)
return f"{self.spec_path.replace(os.path.sep, '.')}{file_portion}{target_portion}{generated_portion}"
prefix = self.spec_path.replace(os.path.sep, ".")
return f"{prefix}{path}{target}{generated}"

def maybe_convert_to_target_generator(self) -> Address:
"""If this address is generated, convert it to its generator target.
Expand Down
25 changes: 21 additions & 4 deletions src/python/pants/build_graph/address_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,13 @@

import pytest

from pants.build_graph.address import Address, AddressInput, InvalidSpecPath, InvalidTargetName
from pants.build_graph.address import (
Address,
AddressInput,
AddressParseException,
InvalidSpecPath,
InvalidTargetName,
)


def test_address_input_parse_spec() -> None:
Expand Down Expand Up @@ -90,15 +96,26 @@ def test_address_input_parse_bad_path_component(spec: str) -> None:
AddressInput.parse(spec)


@pytest.mark.parametrize(
"spec,expected",
[
("a:", "non-empty target name"),
("//:", "non-empty target name"),
("//:@t", "non-empty target name"),
],
)
def test_address_input_parse(spec: str, expected: str) -> None:
with pytest.raises(AddressParseException) as e:
AddressInput.parse(spec)
assert expected in str(e.value)


@pytest.mark.parametrize(
"spec",
[
"",
"a:",
"a::",
"//",
"//:",
"//:@t",
"//:!t",
"//:?",
"//:=",
Expand Down
9 changes: 9 additions & 0 deletions src/python/pants/engine/internals/native_engine.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,15 @@ from pants.engine.process import InteractiveProcessResult
# see https://github.com/psf/black/issues/1548
# flake8: noqa: E302

# ------------------------------------------------------------------------------
# Address (parsing)
# ------------------------------------------------------------------------------

class AddressParseException(Exception):
pass

def address_parse(spec: str) -> tuple[str, str | None, str | None]: ...

# ------------------------------------------------------------------------------
# Scheduler
# ------------------------------------------------------------------------------
Expand Down
8 changes: 8 additions & 0 deletions src/rust/engine/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions src/rust/engine/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ resolver = "2"
# (e.g. fs_util) won't be included when we build/test things.
members = [
".",
"address",
"async_latch",
"async_value",
"cache",
Expand Down Expand Up @@ -60,6 +61,7 @@ members = [
# On Ubuntu, that means installing libfuse-dev. On OSX, that means installing OSXFUSE.
default-members = [
".",
"address",
"async_latch",
"async_value",
"cache",
Expand Down Expand Up @@ -102,6 +104,7 @@ extension-module = ["pyo3/extension-module"]
default = []

[dependencies]
address = { path = "address" }
async_latch = { path = "async_latch" }
# Pin async-trait due to https://github.com/dtolnay/async-trait/issues/144.
async-trait = "=0.1.42"
Expand Down
9 changes: 9 additions & 0 deletions src/rust/engine/address/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
[package]
version = "0.0.1"
edition = "2021"
name = "address"
authors = [ "Pants Build <pantsbuild@gmail.com>" ]
publish = false

[dependencies]
peg = "0.7"
66 changes: 66 additions & 0 deletions src/rust/engine/address/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
// Copyright 2021 Pants project contributors (see CONTRIBUTORS.md).
// Licensed under the Apache License, Version 2.0 (see LICENSE).

#![deny(warnings)]
// Enable all clippy lints except for many of the pedantic ones. It's a shame this needs to be copied and pasted across crates, but there doesn't appear to be a way to include inner attributes from a common source.
#![deny(
clippy::all,
clippy::default_trait_access,
clippy::expl_impl_clone_on_copy,
clippy::if_not_else,
clippy::needless_continue,
clippy::unseparated_literal_suffix,
clippy::used_underscore_binding
)]
// It is often more clear to show that nothing is being moved.
#![allow(clippy::match_ref_pats)]
// Subjective style.
#![allow(
clippy::len_without_is_empty,
clippy::redundant_field_names,
clippy::too_many_arguments
)]
// Default isn't as big a deal as people seem to think it is.
#![allow(clippy::new_without_default, clippy::new_ret_no_self)]
// Arc<Mutex> can be more clear than needing to grok Orderings:
#![allow(clippy::mutex_atomic)]

pub struct AddressInput<'a> {
pub path: &'a str,
pub target: Option<&'a str>,
pub generated: Option<&'a str>,
}

peg::parser! {
grammar relative_address_parser() for str {
rule path() -> &'input str = s:$([^':' | '#']*) {s}

rule target_name() -> &'input str
= quiet!{ s:$([^'#' | '@']+) { s } }
/ expected!("a non-empty target name to follow a `:`.")

rule target() -> &'input str = ":" s:target_name() { s }

rule generated_name() -> &'input str
= quiet!{ s:$([_]+) { s } }
/ expected!("a non-empty generated target name to follow a `#`.")

rule generated() -> &'input str = "#" s:generated_name() { s }

pub(crate) rule relative_address() -> AddressInput<'input>
= path:path() target:target()? generated:generated()? {
AddressInput {
path,
target,
generated,
}
}
}
}

pub fn parse_address(value: &str) -> Result<AddressInput, String> {
let relative_address = relative_address_parser::relative_address(value)
.map_err(|e| format!("Failed to parse Address `{value}`: {e}"))?;

Ok(relative_address)
}
30 changes: 30 additions & 0 deletions src/rust/engine/src/externs/address.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// Copyright 2022 Pants project contributors (see CONTRIBUTORS.md).
// Licensed under the Apache License, Version 2.0 (see LICENSE).

use pyo3::create_exception;
use pyo3::exceptions::PyException;
use pyo3::prelude::*;

use address::parse_address;

create_exception!(native_engine, AddressParseException, PyException);

pub fn register(py: Python, m: &PyModule) -> PyResult<()> {
m.add(
"AddressParseException",
py.get_type::<AddressParseException>(),
)?;
m.add_function(wrap_pyfunction!(address_parse, m)?)?;
Ok(())
}

/// Parses an Address spec into:
/// 1. a path component
/// 2. a target component
/// 3. a generated component
///
#[pyfunction]
fn address_parse(spec: &str) -> PyResult<(&str, Option<&str>, Option<&str>)> {
let address = parse_address(spec).map_err(AddressParseException::new_err)?;
Ok((address.path, address.target, address.generated))
}
1 change: 1 addition & 0 deletions src/rust/engine/src/externs/interface.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ use crate::{

#[pymodule]
fn native_engine(py: Python, m: &PyModule) -> PyO3Result<()> {
externs::address::register(py, m)?;
externs::fs::register(m)?;
externs::nailgun::register(py, m)?;
externs::scheduler::register(m)?;
Expand Down
1 change: 1 addition & 0 deletions src/rust/engine/src/externs/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ use logging::PythonLogLevel;
use crate::interning::Interns;
use crate::python::{Failure, Key, TypeId, Value};

mod address;
pub mod engine_aware;
pub mod fs;
mod interface;
Expand Down

0 comments on commit 397e685

Please sign in to comment.