tmedwards / sugarcube-2

SugarCube is a free (gratis and libre) story format for Twine/Twee.
https://www.motoslave.net/sugarcube/2/
BSD 2-Clause "Simplified" License
185 stars 42 forks source link

<<if>> check for "=" too strict: looks inside of strings #47

Closed boyland closed 4 months ago

boyland commented 4 years ago

Consider the following text

<<set $test to "a=b">>
test: $test
cond: <<= $test.includes("=") >>
<<if $test.includes("=") >>
  Yes!
<</if>>

This generates

test: a=b
cond: true
Error: <<if>>: assignment operator found within <<if>> clause (perhaps you meant to use an equality operator: ==, ===, eq, is), invalid: $test.includes("=")

I ran into this inside of a <<link>> block where the error was not reported (apparently errors are swallowed by <<link>> ?) and so the <<if>> silently failed to do anything, which made the problem much harder to diagnose. (Should <<link>> be swallowing errors?). In any case, I see that there's a configuration option to turn off this behavior for <<if>> but I think it should be fixed anyway.

tmedwards commented 4 years ago

Unfortunately, there is occasionally a need to balance correctness against performance. The <<if>> macro's assignment operator sanity check already drags performance down, especially since <<if>> is so commonly used, so making it even less performant is not something I'm keen on doing.

As you discovered, Config.macros.ifAssignmentError exists to disable the check completely.

That said, I'll look into it.

I ran into this inside of a <<link>> block where the error was not reported (apparently errors are swallowed by <<link>> ?) and so the <<if>> silently failed to do anything, which made the problem much harder to diagnose. (Should <<link>> be swallowing errors?).

Errors should be propagated through <<link>>—though it'll be via a modal. Did you activate the <<link>>? It's contents are not processed until activated.

boyland commented 4 years ago

I see the problem of trying to make the check "smarter" and the fact that it can be turned off is helpful. With regard to <<link>> swallowing errors: no error results when clicking the link -- it simply silently does nothing. Where should the error show up? I do away with the side-bar in generated stories, so perhaps that's why errors end up being silent? The original context where I noticed the problem was:

<<link>>
  <<if condition1>> <<goto "Somewhere">>
  <<elseif condition2>> <<replace "#somewhere">>...<</replace>>
  <<elseif condition3>> <<replace "#somewhere">> ... <</replace>>
  <<else>>
    <<replace "#somewhere">> ... <</replace>>
  <</if>>
<</link>

condition3 was $copy.includes("= newArray") and the entire link had no effect when clicked. Nothing would show up, the <<goto>> didn't happen, no text showed up via <<replace>>. It was very mysterious.

tmedwards commented 4 years ago

Where should the error show up?

As noted in my last reply, the error will be shown in a model window—specifically, a browser alert. Beyond that, it'll also be shown in the console.

I do away with the side-bar in generated stories, so perhaps that's why errors end up being silent?

Using UIBar.destroy() will have no effect on error messages, whether they're displayed in-passage or, as in this case, displayed in a browser modal.

Things to check:


Examples of the browser alert shown below.

Firefox

image

Chrome

image

boyland commented 4 years ago

OK. I'm using Safari. With this example:

:: StoryData
{
    "ifid": "71F15800-A142-4801-BD59-A8CDA7A65D43",
    "format": "SugarCube",
    "format-version": "2.30.0"
}

:: StoryTitle
Testing a bad if in a link

:: Start
This is the starting passage of a story.
<<link "Test">>
 <<set $test to "a=b">>
test: $test
cond: <<= $test.includes("=") >>
<<if $test.includes("=") >>
  Yes!
<</if>>
<</link>>

It comes up without an error. Pressing the "Test" link has no visible effect, but when I open the JavaScript console, I see the error:

[Error] Not allowed to load local resource: file://null/Users/boyland/Documents/Eclipse/runtime-EclipseApplication/twee-project/twee/es6-shim.map
[Error] Not allowed to request resource
[Error] Cannot load file://null/Users/boyland/Documents/Eclipse/runtime-EclipseApplication/twee-project/twee/es6-shim.map due to access control checks.
[Warning] Error: <<if>>: assignment operator found within <<if>> clause (perhaps you meant to use an equality operator: ==, ===, eq, is), invalid: $test.includes("=")  (testlinkif.html, line 114)
    <<if $test.includes("=") >>
      Yes!
    <</if>>
[Error] Error: <<if>>: assignment operator found within <<if>> clause (perhaps you meant to use an equality operator: ==, ===, eq, is), invalid: $test.includes("=") 
    value (testlinkif.html:117:13580)
    (anonymous function) (testlinkif.html:118:20209)
    (anonymous function) (testlinkif.html:115:505)
    dispatch (testlinkif.html:59:42577)
[Error] TypeError: null is not an object (evaluating 'n.stack')
    e (testlinkif.html:114:1754)
    t (testlinkif.html:114:1831)
    (anonymous function) (testlinkif.html:114:1967)

So moral of the story: When I'm debugging things, I really need to look at the JavaScript console.

boyland commented 4 years ago

BTW: Some errors show up similarly to the Chrome example you give: these show up when I first load the file into Safari but not afterwards.

tmedwards commented 4 years ago

If any of those startup errors mention being fatal and aborting—i.e., (emphasis added) "Apologies! A fatal error has occurred. Aborting."—then that's your problem wrt. seeing additional error alerts. Fatal errors are not considered recoverable and the system pretty much gives up at that point.

If you're not seeing any fatal errors, then I'm not sure what it could be. Safari's on track to become the new IE6, so you might want to try a less derpy browser to see if that makes a difference.