Skip to content

fix: preserve literal namespace components across evaluation paths - #827

Open
Maksym (maksym-mishchenko) wants to merge 2 commits into
microsoft:mainfrom
maksym-mishchenko:maksym-mishchenko-dotted-namespace-compatibility
Open

Maksym (maksym-mishchenko) wants to merge 2 commits into
microsoft:mainfrom
maksym-mishchenko:maksym-mishchenko-dotted-namespace-compatibility

Conversation

@maksym-mishchenko

Copy link
Copy Markdown
Contributor

Summary

I preserve literal string components in canonical package and rule paths, so graph.defUniqueName["1.0.0"] remains distinct from nested namespaces. Canonical paths work across metadata, direct and compiled interpreter evaluation, and RVM without ambiguous flattened aliases.

The regressions cover defaults and Undefined, escaped components, imports and functions, dynamic lookup, numeric-key precedence and mixed base/virtual data, long configured target paths, serialization, and reused execution in both modes. Public signatures and serialized program layout stay unchanged; related grammar and API documentation describe the canonical notation.

Validation

  • Final local cargo xtask ci-debug --frozen, formatting and strict Clippy passed.
  • Fresh prepublication checks passed 12 namespace tests, including 14 fixture cases, plus the long target and existing Azure policy cases.
  • The unchanged pre-commit/pre-push validators passed, including builds, formatting, strict Clippy, docs, no_std, ACI/Kata, extensions and 2,875 pinned OPA fixtures. Approved task-local Windows launchers preserved failure propagation and were removed afterward; shared hooks and configuration were not changed.

Evidence limits

Native OPA parity remains unverified; pinned OPA v1.2.0 fixtures are not a native comparison. Shared FFI and C# runtime tests passed earlier in local acceptance but were not rerun after the last two isolated fixes. No latest-DLL or all-nine binding runtime validation is claimed. The unsupported no-default rvm,arc without std combination remains an unchanged baseline exception.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@anakrish

Copy link
Copy Markdown
Collaborator

Maksym (@maksym-mishchenko) Can you add a test of integer/number package components e.g. "1", "1.0". IIRC, OPA has some strange semantics around those, in the package name context or as part of input.

@anakrish

Copy link
Copy Markdown
Collaborator

Maksym (@maksym-mishchenko)

Can you back up your current branch and try a small prototype of the following:

As Regorus evolved, there is bit of path related logic has become scattered, and this PR is a good opportunity to centralize it.

We could introduce structural path types, roughly along these lines:

#[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
pub(crate) enum StaticPathComponent {
    /// `.foo` and `["foo"]` have the same structural identity.
    String(Rc<str>),
    Number(Number),
    Bool(bool),
    Null,
}
#[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
pub(crate) struct StaticPath {
    components: Box<[StaticPathComponent]>,
}
/// Components declared after `package`; excludes the implicit `data` root.
#[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
pub(crate) struct PackagePath(StaticPath);
/// Complete, statically known rule identity, rooted at `data`.
#[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
pub(crate) struct RulePath(StaticPath);
impl PackagePath {
    fn from_ref(expr: &Expr) -> Result<Self>;
    fn parse(text: &str) -> Result<Self>;
    fn to_rule_path(&self, rule: &StaticPath) -> Result<RulePath>;
    fn canonical(&self) -> Result<String>;
}
impl RulePath {
    fn parse(text: &str) -> Result<Self>;
    fn canonical(&self) -> Result<String>;
    fn parent(&self) -> Option<Self>;
    fn starts_with(&self, prefix: &Self) -> bool;
    fn components(&self) -> &[StaticPathComponent];
}

The above types would capture static paths; dynamic paths can be addressed later.

Could you see how much of the minimum logic needed for this PR can be moved behind types like these? If the change remains reasonably contained, I would strongly prefer the typed approach. If it expands the scope too much, we can retain the current implementation for this PR and use the prototype as something to be revisited later.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@maksym-mishchenko

Copy link
Copy Markdown
Contributor Author

I loaded packages p["1"] and p["1.0"] together. They stayed separate in both the interpreter and VM. With string-keyed data, numeric 1 and 1.0 lookups hit "1"; numeric lookups on a plain input object were undefined. I haven’t checked OPA yet.

@maksym-mishchenko

Copy link
Copy Markdown
Contributor Author

I put the package and rule-path parsing behind typed path objects in the prototype. Parsing, formatting, and prefix checks passed for string, number, boolean, and null components. Rule storage still uses string keys, so I’d leave that larger change out of this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants