Skip to content

Comments

parser.y: properly abstract over to-be-created strings#14644

Merged
Radvendii merged 2 commits intoNixOS:masterfrom
Radvendii:fix-14642
Nov 25, 2025
Merged

parser.y: properly abstract over to-be-created strings#14644
Radvendii merged 2 commits intoNixOS:masterfrom
Radvendii:fix-14642

Conversation

@Radvendii
Copy link
Contributor

This fixes #14642 and adds tests to prevent regression.

In eab467e, I had missed cases like "${"string"}", where the Expr is already an ExprString. This PR provides a wrapper around std::variant that correctly abstracts over when we have a string and when we have another Expr.

  • parser.y: correctly abstract over to-be-constructed ExprString
  • tests: add tests for dynamic attribute in let and inherit

Motivation

Context


Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@Radvendii Radvendii requested a review from edolstra as a code owner November 25, 2025 16:53
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Nov 25, 2025
@xokdvium xokdvium self-assigned this Nov 25, 2025
@Ericson2314
Copy link
Member

Looks pretty good to me, but needs a formatter run.

Copy link
Contributor

@xokdvium xokdvium left a comment

Choose a reason for hiding this comment

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

One minor comment. Also would like to see more tests. I can take over if that's fine with you @Radvendii

@Radvendii Radvendii force-pushed the fix-14642 branch 2 times, most recently from 0c6ba70 to ee2277e Compare November 25, 2025 19:26
@Radvendii
Copy link
Contributor Author

Radvendii commented Nov 25, 2025

I added a few more tests (some results still surprised me. inherit ${"foo"}; is valid but inherit "${foo"}"; is not).

You're also welcome to add more of course.

One problem is that this is such cursed nix code that the formatter doesn't know what to do with it and CI is failing.

  1. does this count as a bug in the formatter that I should send their way? (the code it's failing to format doesn't succeed to parse but for a reason other than the one the formatter is giving)
  2. can we disable the formatter for specific files?

Radvendii and others added 2 commits November 25, 2025 23:33
Fixes the regression from eab467e with
dynamic attributes that a simple string expressions.

Co-authored-by: Sergei Zimmerman <sergei@zimmerman.foo>
Regression tests for the previous commit.

Co-authored-by: Sergei Zimmerman <sergei@zimmerman.foo>
Co-authored-by: piegames <git@piegames.de>
Copy link
Contributor

@xokdvium xokdvium left a comment

Choose a reason for hiding this comment

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

Added a bit more tests (example from @piegamesde) and clarified some comments, added monostate instead of default initialized empty std::string_view.

@Radvendii Radvendii added this pull request to the merge queue Nov 25, 2025
Merged via the queue into NixOS:master with commit 952be9f Nov 25, 2025
16 checks passed
@Radvendii Radvendii deleted the fix-14642 branch November 25, 2025 23:19
@flokli
Copy link
Member

flokli commented Nov 29, 2025

@Ericson2314 do you plan to tag a release containing this fix?

@Ericson2314
Copy link
Member

@flokli I haven't been involved with point releases very much at all, but yes that sounds like a good idea to me. I will raise it with the others.

@edolstra edolstra mentioned this pull request Dec 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

with-tests Issues related to testing. PRs with tests have some priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"dynamic attributes not allowed" in let regression

4 participants