Bug Directives are allowed at the start of any block (ASH)

fredg1

Member
On top of being allowed at the start of files, directives (script, notify, since, import) are currently allowed at the start of any "block" (something that starts with a { and ends with a }, other than array literals).

I believe this to be a bug/unintended behaviour, and here's why:

script & notify​

Only the first one to appear (from top to bottom) does anything.
This means there is literally no purpose to placing those at anywhere but the top of the file (as intended).
java_XrCofVF8U8.png

since​

Every single one of them gets parsed, with no way to choose which.
This means that, again, there is literally no purpose in placing a single `since` directive at the start of the file.
notepad++_zjArrwvg7s.png

import​

Only the first instance of an `import` (from top to bottom) gets added to the script.
This is just clearly an unintended behaviour...
notepad++_4X2RLeGL1q.pngnotepad++_3plHcW2tZr.png


I believe that this is sufficiently obscure (and, frankly, useless) to be removed/changed without consequences.

Attached to this is a patch that I believe would solve this issue.
 

Attachments

  • directives.patch
    2 KB · Views: 4

gausie

D̰͕̝͚̤̥̙̐̇̑͗̒e͍͔͎͈͔ͥ̉̔̅́̈l̠̪̜͓̲ͧ̍̈́͛v̻̾ͤe͗̃ͥ̐̊ͬp̔͒ͪ
Staff member
I agree that script needn't be given any attention if redeclared, though I can see an argument for notify

That since behaviour is probably preferable to removing support for now - i.e. we enforce the highest since value present in the code.

Obviously in an ideal world you could actually use import like that.

I think it might make more sense to mark these as "warnings" if possible for now, just to validate that they can be removed without consequence at a later stage.

In any case I would check is that your patch doesn't break one file with a since directive importing another file with a since directive, because they're sort of concatenated.
 

fredg1

Member
though I can see an argument for notify
No. A script can only notify a single user, so more than one notify does nothing.
That since behaviour is probably preferable to removing support for now - i.e. we enforce the highest since value present in the code.
There's no issue with that. The issue is how weird it would be to add a since in the middle of your code.
Your script can have an since directive, and your imported scripts can each have one, too. Whichever is highest doesn't even matter; it's enforced immediately upon seeing one/them/each/any.
In any case I would check is that your patch doesn't break one file with a since directive importing another file with a since directive, because they're sort of concatenated.
import directive don't actually concatenate the content of the imported script in the importing script; the "main" script, as well as each imported script, can all have their own directives without problems.
(Either way, you can give a quick look at the patch to see that this was, indeed, taken into consideration)
 

gausie

D̰͕̝͚̤̥̙̐̇̑͗̒e͍͔͎͈͔ͥ̉̔̅́̈l̠̪̜͓̲ͧ̍̈́͛v̻̾ͤe͗̃ͥ̐̊ͬp̔͒ͪ
Staff member
You're not reading my message properly and you're being rude. I will not be looking at this thread anymore
 

fredg1

Member
In short: surprising and undocumented behavior that arises from implementation quirks is error-prone and should not be part of the contract.
@heeheehee while we're talking about undocumented and unintended behaviour.
Updated the patch to be compatible with the current version.

(I still, to this day, have no idea what gausie was upset about...)
 

Attachments

  • directives_2.patch
    4 KB · Views: 0

heeheehee

Developer
Staff member
I do see gausie's point that this change has significant potential to break existing scripts, and there are arguments for different bits of behavior in the future.

For instance, many other languages' import pulls in a module, instead of polluting the global namespace. Or you might want to have multiple since statements to enforce that different bits of functionality exist (or, check for a release and a revision).

In your response to gausie, you come across as dismissive of his concerns, and then you basically say "you can look at the patch to confirm that it works." Bear in mind that we're all doing this in our free time, and reviewing a patch takes time to simply understand what it's doing (let alone making sure it doesn't break anything, or confirming that it actually does what it's supposed to), especially when not already familiar with that chunk of the codebase.
 

fredg1

Member
In your response to gausie, you come across as dismissive of his concerns, and then you basically say "you can look at the patch to confirm that it works."
If I came across as dismissive, I would welcome help to not appear as such, but my intent wasn't to be dismissive, it was simply to progress the discussion.
I wasn't trying to say "naaaaah, you're worrying for nothing, c'mon, commit!!", I was addressing his points as best as I understood them, and was letting him respond.
I successfully addressed a point? Great, he just won't need to mention it again.
I misunderstood something? You can point it out.
I only partially addressed something, or addressed something unrelated/unsubstantial? Then bring it up, and continue talking about the point.

Point per point:

though I can see an argument for notify
No. A script can only notify a single user, so more than one notify does nothing.
I didn't really understand what he meant by "I can see an argument for it". He didn't explain what the argument was. As far as I knew, there was no use to a second notify argument, because they were ignored altogether, so I simply pointed it out. If I was wrong, he could have replied with something like "uh... Yes, <explanation of a way a second notify directive matters>", and I would have felt stupid for not thinking about it (if it was true).

That since behaviour is probably preferable to removing support for now - i.e. we enforce the highest since value present in the code.
There's no issue with that. The issue is how weird it would be to add a since in the middle of your code.
Your script can have an since directive, and your imported scripts can each have one, too. Whichever is highest doesn't even matter; it's enforced immediately upon seeing one/them/each/any.
Here, I wasn't dismissive, I was literally agreeing with him.
I only noted that it would "feel weird" to see that directive in the middle of a script.
Does he agree? Does he disagree? Should we keep allowing these anywhere? I was just... waiting to find out, to see where to go next...

In any case I would check is that your patch doesn't break one file with a since directive importing another file with a since directive, because they're sort of concatenated.
import directive don't actually concatenate the content of the imported script in the importing script; the "main" script, as well as each imported script, can all have their own directives without problems.
(Either way, you can give a quick look at the patch to see that this was, indeed, taken into consideration)
Here, I just... didn't understand what he meant??
No, as far as I know, there's no such thing as the directives being "sort of concatenated"?
I pointed out that, AFAIK, that point was false, since it was resting on a false premise (as I understood it), and that he could see for himself, because there's no other way to prove this: you can argue as much as you want, swear that something behaves one way or another, the only way to be sure is to... see it for yourself.

This is the same thing as the first point: had I missed something obvious? He could have pointed it out, and could even be entitled to lord it over me.
 

fredg1

Member
Keep in mind, at that time, we were up 12 hours apart, so we were only exchanging one message per day, so a simple "could you expand on that?" would have been a waste of 24 hours.
I was being efficient, by addressing everything as well as I could, until the next exchange, which I was patiently waiting for.
 
Gausie may have been suggesting that there is an argument for allowing scripts to notify more than one user. Given that there are quite a few scripts that have been taken over by a community, it may be good, but I don't know if it outweighs the reasons it was originally limited to one.

No comment on the rest of it.
 

heeheehee

Developer
Staff member
My intentionally uncharitable line-by-line read of that post:
No. A script can only notify a single user, so more than one notify does nothing.
"No, there is no reason for doing so. It doesn't work that way."

(Alt: "As currently implemented, all `notify` directives after the first one are ignored. Can you say more about what you had in mind?" Or: "Since subsequent notify directives are silently ignored, this can be confusing to script authors, and error-prone.")
There's no issue with that. The issue is how weird it would be to add a since in the middle of your code.
"That's a non-issue. What you should be worried about instead is this weird construction."
Your script can have an since directive, and your imported scripts can each have one, too. Whichever is highest doesn't even matter; it's enforced immediately upon seeing one/them/each/any.
(By highest, gausie likely meant that enforcing all of the `since` directives was effectively equivalent to enforcing the highest, i.e. strictest, one.)
import directive don't actually concatenate the content of the imported script in the importing script; the "main" script, as well as each imported script, can all have their own directives without problems.
"You're wrong. That's not how import works."

(Alt: "As I understand it, import in ASH creates a separate Parser instance for each imported script, then adds all of the discovered top-level functions to the global namespace, as opposed to languages like C where #include directly inserts the contents of the included file." -- In particular, giving some context is more valuable than just saying "no you're wrong.")
(Either way, you can give a quick look at the patch to see that this was, indeed, taken into consideration)
"If you don't trust me, fine, spend the next 20 minutes of your time working through this patch."

Now, don't get me wrong: I'm not saying that you wrote your response while actively trying to be rude. I'm just showing how it could have been interpreted that way.

Part of the issue overall is that you cherry-picked several aspects of his post, while not responding to the more salient points / suggestions: make these warnings for now, and test that you're not breaking behavior (e.g. with the sample test case he provided).

Unfortunately, conciseness can often be interpreted as rudeness, especially so in virtual environments where you can't lean on tone or body language to suggest otherwise. This is especially amplified if you use absolute terms ("No." "That's not how that works." "You're wrong.")

There's nothing wrong with responding point-by-point for the sake of efficiency, but if you don't understand something, then ask for clarification, rather than deciding on an interpretation or even outright ignoring the feedback.
 
Top