Invertible attributes #2393

neunenak · 2024-09-27T07:35:42Z

This PR implements a not() syntax for certain OS-target attributes.

Resolves #1895

neunenak · 2024-12-19T04:54:34Z

@casey this is ready for review

casey

See some initial comments!

casey · 2024-12-22T02:45:15Z

src/attribute_set.rs

+            InvertedStatus::Normal
+          }
+        }
+        _ => return None,


I think this function should panic if we call it with a non-invertible attribute.

casey · 2024-12-22T02:58:47Z

src/recipe.rs

+    };
+
+    if cfg!(target_os = "linux") {
+      return !(disabled.linux || disabled.unix)


Are these unix checks redundant with the ones below? I notice that the other Unix OS's don't have the same checks.

I think this maybe all gets easier if we have a System enum which represents the current system:

enum System { Windows, Linux, Unix, ... } impl System { fn current() -> &'static [System] { if cfg(target_os = "linux") { &[Self::Linux, Self::Unix] } } }

And then we can handle all these checks in the same way.

casey · 2024-12-22T03:03:58Z

src/recipe.rs

-      || (cfg!(target_os = "windows") && windows)
-      || (cfg!(unix) && unix)
-      || (cfg!(windows) && windows)
+    use attribute_set::InvertedStatus;


I'm not sure I like introducing inverted status only to turn it back to a bool with matches!. I think it would probably be better to just use a bool the whole way.

casey · 2024-12-22T03:04:50Z

src/attribute.rs

@@ -13,17 +13,17 @@ pub(crate) enum Attribute<'src> {
  Doc(Option<StringLiteral<'src>>),
  Extension(StringLiteral<'src>),
  Group(StringLiteral<'src>),
-  Linux,
-  Macos,
+  Linux { inverted: bool },


I think we should probably use:

Suggested change

Linux { inverted: bool },

Linux { enabled: bool },

And invert the meaning of the boolean, to make the downstream code make more sense.

casey · 2024-12-22T03:05:07Z

src/attribute.rs

-      AttributeDiscriminant::Private => Self::Private,
-      AttributeDiscriminant::Script => Self::Script({
+    Ok(match (inverted, discriminant) {
+      (inverted, AttributeDiscriminant::Linux) => Self::Linux { inverted },


Suggested change

(inverted, AttributeDiscriminant::Linux) => Self::Linux { inverted },

(inverted, AttributeDiscriminant::Linux) => Self::Linux { enabled: !inverted },

casey · 2024-12-22T03:07:16Z

src/parser.rs

@@ -1135,7 +1135,18 @@ impl<'run, 'src> Parser<'run, 'src> {
      token.get_or_insert(bracket);

      loop {
-        let name = self.parse_name()?;
+        let (name, inverted) = {


I think this is probably cleaner than using mutable variables:

Suggested change

let (name, inverted) = {

let (name, inverted) = {

let name = self.parse_name()?;

if name.lexeme() == "not" {

self.expect(ParenL)?;

let name = self.parse_name()?;

(name, true)

} else {

(name, false)

}

}

And I think we should actually, since we don't have any invertable variables which aren't system variables, change the argument to Attribute::new to be !inverted, and within Attribute::new it's called enabled.

casey

See comments. This looks great, I like the structure, but we definitely need to do some kind of more extensive testing.

casey · 2025-01-03T21:43:22Z

src/recipe.rs

+    if cfg!(unix) {
+      return Unix;
+    }
+    panic!("No recognized system");


I don't think this can panic. We should have an Unrecognized variant.

casey · 2025-01-03T21:47:58Z

src/recipe.rs

+    panic!("No recognized system");
+  }
+
+  fn enabled(self, enabled: SystemMap, disabled: SystemMap) -> bool {


I think we need some kind of more extensive testing for this. It looks right to me, but it strikes me as very fragile, and easy to get wrong when extending..

Yeah, makes senses - I'm not not entirely happy with the fragility of this code either, but I don't have any ideas off the top of my head for how to make it more robust. Do you have any ideas for how the tests should be architected?

One idea:

#[test] fn foo() { fn case(attributes: &str, active: bool) { Test::new().justfile(format!("[{attributes}]\nfoo:")) // assert that foo is or isn't active, depending on attributes. } case("macos", cfg!(target_os = "darwin")); }

One idea for improving the structure would be to try to factor out this pattern:

System::Windows => { !disabled.windows && (enabled.windows || !(enabled.macos || enabled.linux || enabled.openbsd || enabled.unix)) }

Maybe with maps:

!disabled[self] && (enabled[self] || !self.others().iter().any(|other| system.enabled()))

casey · 2025-01-03T21:49:30Z

src/recipe.rs

+          && (enabled.windows
+            || !(enabled.macos || enabled.linux || enabled.openbsd || enabled.unix))
+      }
+      System::MacOS => {


On MacOS, and other unix systems, a recipe with [not(unix)] will be enabled. This seems wrong to me.

casey#1895 Swap inverted to enabled Use System type

neunenak force-pushed the invertable-attribute branch from 9dc142a to 1c69b18 Compare September 28, 2024 19:49

neunenak mentioned this pull request Oct 8, 2024

Add AttributeSet type #2419

Merged

neunenak force-pushed the invertable-attribute branch 7 times, most recently from c36db49 to 148542b Compare December 18, 2024 02:01

neunenak marked this pull request as ready for review December 18, 2024 02:01

neunenak changed the title ~~WIP Invertable attributes~~ Invertible attributes Dec 18, 2024

casey requested changes Dec 22, 2024

View reviewed changes

neunenak force-pushed the invertable-attribute branch from 7d30976 to 78c67e2 Compare December 24, 2024 04:38

neunenak force-pushed the invertable-attribute branch 2 times, most recently from c3f4feb to 1bacd8c Compare December 31, 2024 07:48

neunenak requested a review from casey December 31, 2024 07:48

casey requested changes Jan 3, 2025

View reviewed changes

neunenak force-pushed the invertable-attribute branch 4 times, most recently from bfd54da to 8362ffc Compare March 12, 2025 22:08

neunenak added 4 commits April 7, 2025 18:52

not() attribute

4f230ce

casey#1895 Swap inverted to enabled Use System type

Remove panic

010e3cc

refactor

88335fe

AI fix?

bfba342

neunenak force-pushed the invertable-attribute branch from 8362ffc to bfba342 Compare April 8, 2025 01:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Invertible attributes #2393

Invertible attributes #2393

neunenak commented Sep 27, 2024 •

edited

Loading

neunenak commented Dec 19, 2024

casey left a comment

casey Dec 22, 2024

casey Dec 22, 2024

casey Dec 22, 2024

casey Dec 22, 2024

casey Dec 22, 2024

casey Dec 22, 2024

casey Dec 22, 2024

casey Dec 22, 2024

casey left a comment

casey Jan 3, 2025

casey Jan 3, 2025

neunenak Jan 8, 2025

casey Jan 8, 2025 •

edited

Loading

casey Jan 3, 2025

	(inverted, AttributeDiscriminant::Linux) => Self::Linux { inverted },
	(inverted, AttributeDiscriminant::Linux) => Self::Linux { enabled: !inverted },

-        let (name, inverted) = {
+        let (name, inverted) = {
+          let name = self.parse_name()?;
+          if name.lexeme() == "not" {
+            self.expect(ParenL)?;
+            let name = self.parse_name()?;
+            (name, true)
+          } else {
+            (name, false)
+          }
+        }

Invertible attributes #2393

Are you sure you want to change the base?

Invertible attributes #2393

Conversation

neunenak commented Sep 27, 2024 • edited Loading

neunenak commented Dec 19, 2024

casey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

casey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

casey Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neunenak commented Sep 27, 2024 •

edited

Loading

casey Jan 8, 2025 •

edited

Loading