Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

parser-state: fix attribute merging #11294

Merged
merged 1 commit into from
Nov 27, 2024
Merged

Conversation

rhendric
Copy link
Member

Motivation

Fixes #11268.

Context

This should be non-breaking. As a side effect, it makes some duplicate attribute error messages nicer (later occurrences point back to previous occurrences rather than the other way around).

I'm working at the edge of my proficiency with C++ here, so please take extra care to verify that how I'm handling pointers and memory management is sensible, because I'm not confident that I'm doing it right.

Priorities and Process

Add 👍 to pull requests you find important.

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

@rhendric rhendric requested a review from edolstra as a code owner August 14, 2024 04:18
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Aug 14, 2024
@@ -189,6 +155,60 @@ inline void ParserState::addAttr(ExprAttrs * attrs, AttrPath && attrPath, const
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know if this stuff should be moved to the new overload or if it should be left here; I don't really understand what it's doing.

Copy link
Member

Choose a reason for hiding this comment

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

What stuff are you referring to?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I was referring to the above five lines involving lexerState.positionToDocComment; it's the same ‘stuff’ I'm wondering about in #11294 (comment).

Comment on lines +191 to +201
ae->attrs.clear();
jAttrs->dynamicAttrs.insert(jAttrs->dynamicAttrs.end(),
std::make_move_iterator(ae->dynamicAttrs.begin()),
std::make_move_iterator(ae->dynamicAttrs.end()));
ae->dynamicAttrs.clear();
if (ae->inheritFromExprs) {
jAttrs->inheritFromExprs->insert(jAttrs->inheritFromExprs->end(),
std::make_move_iterator(ae->inheritFromExprs->begin()),
std::make_move_iterator(ae->inheritFromExprs->end()));
ae->inheritFromExprs = nullptr;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

At the end of this block, everything about ae worth retaining should have been merged into jAttrs. In principle I would think that means ae can be deleted. But because of the doc comment stuff lingering in the other overload, I don't think it's safe to do that. I settled for just clearing vectors instead. (I'm guessing at the right non-leaky stuff to do here, in general; are these move_iterators used correctly?)

I'm confused, though, about what good that doc comment stuff can do if it's being attached to an expression that essentially gets dropped on the floor.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe @roberth knows?

@rhendric rhendric force-pushed the rhendric/fix-11268 branch from c780262 to 66ae0e2 Compare August 14, 2024 09:33
Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

Wow! :)

src/libexpr/parser-state.hh Outdated Show resolved Hide resolved
@tomberek tomberek added the idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. label Aug 19, 2024
@rhendric rhendric force-pushed the rhendric/fix-11268 branch from 66ae0e2 to 85b7453 Compare August 20, 2024 22:08
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-08-19-nix-team-meeting-minutes-170/50942/1

@roberth
Copy link
Member

roberth commented Aug 26, 2024

Eelco has the week off. He'll review this later.

Copy link

@kh3rld kh3rld left a comment

Choose a reason for hiding this comment

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

The improvements to error messaging are great!!

@Mic92
Copy link
Member

Mic92 commented Sep 20, 2024

@edolstra is back afaik.

tests/unit/libexpr/trivial.cc Outdated Show resolved Hide resolved
src/libexpr/parser-state.hh Show resolved Hide resolved
tests/unit/libexpr/trivial.cc Outdated Show resolved Hide resolved
@rhendric rhendric force-pushed the rhendric/fix-11268 branch from 85b7453 to 05515c5 Compare October 3, 2024 00:20
@Mic92 Mic92 requested a review from edolstra November 24, 2024 10:12
@edolstra
Copy link
Member

@rhendric Can you resolve the merge conflict? Thanks!

@Mic92 Mic92 force-pushed the rhendric/fix-11268 branch from 05515c5 to 8034589 Compare November 27, 2024 20:45
@Mic92 Mic92 enabled auto-merge November 27, 2024 20:50
@Mic92
Copy link
Member

Mic92 commented Nov 27, 2024

I rebased. There was no actual merge conflict.

@Mic92 Mic92 merged commit 5756caf into NixOS:master Nov 27, 2024
17 checks passed
@rhendric rhendric deleted the rhendric/fix-11268 branch November 28, 2024 05:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. with-tests Issues related to testing. PRs with tests have some priority
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Nested attribute merging may or may not work as intended
10 participants