Skip to content
New issue

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

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

Already on GitHub? # to your account

[FP] Avoid linting for $$ with &convert #54

Merged
merged 1 commit into from
Nov 4, 2024

Conversation

evantypanski
Copy link
Contributor

@evantypanski evantypanski commented Oct 30, 2024

Attributes on a field with &convert make $$ mean something different. $$ refers to before the conversion, self.field refers to after.

Obligatory note: There's more nuance, but I almost see it as a spicy bug. If you run the example and print both self.x and $$, you'll see they'll be the same. But if you try swapping them out in a &requires, they'll be different. That seems wrong to me, so I didn't add that logic in. Maybe worth an issue

BTW this is where it was triggering in spicy Redis, where the replacement makes Spicy not compile the code because that's before a type conversion:

https://github.com/evantypanski/spicy-redis/blob/8101fd33f1c3b3e627e6032741c3c42126790670/analyzer/resp.spicy#L108

Attributes on a field with `&convert` make `$$` mean something
different. $$ refers to before the conversion, `self.field` revers to
after.
Copy link
Owner

@bbannier bbannier left a comment

Choose a reason for hiding this comment

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

Thanks, this makes sense. Regarding your example

   num_elements: RedisBytes &convert=$$.to_int(10) &requires=self.num_elements <= int64(MAX_SIZE);

I am not sure this is exactly a bug. The Spicy docs document this as

Inside field attributes and hooks, $$ refers to the just parsed value, even if it’s not going to be directly stored in the field. The value of $$ is writable and may be modified.

I agree that the interaction with a following &requires is probably surprising, but it seems to work as designed (we disallow multiple &convert calls). I just quickly checked whether this could misbehave with if $$ is written but this also seems to work.

@bbannier bbannier merged commit 038acb0 into bbannier:main Nov 4, 2024
2 checks passed
@evantypanski evantypanski deleted the topic/etyp/convert-dd branch November 4, 2024 14:57
@evantypanski
Copy link
Contributor Author

I am not sure this is exactly a bug. The Spicy docs document this as

I'm not too sure either for this case, but I was referring to the fact that $$ refers to something different in &requires (before the &convert) versus in the block (after the &convert). My intuition is $$ should always refer to the pre-conversion value, even in the block. Instead, $$ seems to refer to 2 different things for the same field depending on where it's called. That confused me a fair bit, but it behaves consistently from what I can tell.

@bbannier
Copy link
Owner

bbannier commented Nov 4, 2024

I am not sure this is exactly a bug. The Spicy docs document this as

I'm not too sure either for this case, but I was referring to the fact that $$ refers to something different in &requires (before the &convert) versus in the block (after the &convert). My intuition is $$ should always refer to the pre-conversion value, even in the block. Instead, $$ seems to refer to 2 different things for the same field depending on where it's called. That confused me a fair bit, but it behaves consistently from what I can tell.

Ah, that's not good. Would you mind filing a Spicy ticket for that?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants