Bug 151503 - logical operator semantics on missing (optional) arguments has changed between 7.2 and 7.4
Summary: logical operator semantics on missing (optional) arguments has changed betwee...
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: BASIC (show other bugs)
Version:
(earliest affected)
7.4.1.2 release
Hardware: All All
: high major
Assignee: Andreas Heinisch
URL:
Whiteboard: target:7.5.0 target:7.4.3
Keywords:
Depends on:
Blocks:
 
Reported: 2022-10-13 08:19 UTC by Lionel Elie Mamane
Modified: 2023-01-21 16:00 UTC (History)
5 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Lionel Elie Mamane 2022-10-13 08:19:00 UTC
Description:
Consider the following code:


  sub tst
    tstOpt
  end sub

  sub tstOpt(optional foo as boolean)
	if ismissing(foo) or foo then
	   msgbox "yes"
	else
	   msgbox "no"
	end if
  end sub


In LibreOffice 7.2, calling sub tst (and thus calling tstOpt without its optional argument) would print "yes". I'm not sure whether a missing parameter was coerced to true by the "or" operator, or if the "or" operator was evaluating its arguments lazily left to right, or both.

In LibreOffice 7.4, this gives an error "Argument is not Optional".

This breaks user code.

Compare with:

  sub tstOpt2(optional foo as boolean)
	if foo then
	   msgbox "yes"
	else
	   msgbox "no"
	end if
  end sub

Which shows "yes" in both LibO 7.2 and LibO 7.4, showing that the if/then construct does coerce a missing parameter to true.

The same happens with "and" and "not" operators.

Probably the logical operators and/not/or/... should continue to do the same for backwards compatibility and coherence?

Steps to Reproduce:
run sub tst

Actual Results:
error "Argument is not Optional"

Expected Results:
messagebox showing "Yes"


Reproducible: Always


User Profile Reset: No



Additional Info:
Version: 7.4.1.2 / LibreOffice Community
Build ID: 3c58a8f3a960df8bc8fd77b461821e42c061c5f0
CPU threads: 8; OS: Linux 5.10; UI render: default; VCL: gtk3
Locale: fr-LU (fr_LU.UTF-8); UI: en-GB
Calc: threaded
Comment 1 Mike Kaganski 2022-10-13 09:07:46 UTC
(In reply to Lionel Elie Mamane from comment #0)
> Compare with:
> 
>   sub tstOpt2(optional foo as boolean)
> 	if foo then
> 	   msgbox "yes"
> 	else
> 	   msgbox "no"
> 	end if
>   end sub
> 
> Which shows "yes" in both LibO 7.2 and LibO 7.4

I would consider this a bug. It must give the "not optional/missing" error.

In Basic, there's no boolean short circuiting (unfortunately), so accessing a missing argument "after" checking IsMissing in the same expression is not valid.
Comment 2 Andreas Heinisch 2022-10-13 09:23:40 UTC
Confirmed in:
Version: 7.5.0.0.alpha0+ (x64) / LibreOffice Community
Build ID: d1707bc31261d16893c1f5240c803d283e293ec1
CPU threads: 16; OS: Windows 10.0 Build 19044; UI render: Skia/Raster; VCL: win
Locale: de-DE (de_DE); UI: en-US
Calc: CL threaded

Excel on the other hand displays "no".

Imho, the expected behaviour should be the "not optional/missing" error message.
Comment 3 Mike Kaganski 2022-10-13 09:29:46 UTC
(In reply to Andreas Heinisch from comment #2)
> Excel on the other hand displays "no".

This is unrelated, since in VBA, (and in our VBA mode) the non-variant optionals are initialized by the default value of the type.
Comment 4 Xisco Faulí 2022-10-13 10:02:09 UTC
Regression introduced by:

author	Andreas Heinisch <andreas.heinisch@yahoo.de>	2021-09-07 22:23:43 +0200
committer	Andreas Heinisch <andreas.heinisch@yahoo.de>	2021-09-10 13:28:33 +0200
commit e32d864dbe086d630a8b17c2d376e320aee0253a (patch)
tree 75637151dd3e02af879ffceba0427ca4287f4d2c
parent 66e9b77ed296bb6b0cdae22685a11525a06dcf08 (diff)
tdf#144353 - Handling of missing optional parameters

Bisected with: bibisect-linux64-7.3

Adding Cc: to Andreas Heinisch
Comment 5 Lionel Elie Mamane 2022-10-13 11:06:08 UTC
(In reply to Mike Kaganski from comment #1)
> I would consider this a bug. It must give the "not optional/missing" error.

(In reply to Andreas Heinisch from comment #2)
> Imho, the expected behaviour should be the "not optional/missing" error
> message.

Your common position makes sense in the absolute, but it is an incompatible change that breaks user code :-(
Comment 6 Mike Kaganski 2022-10-13 11:15:01 UTC
Because it is not a regression. The change was intentional, and fixes a pre-existing bug that you could access a missing parameter. The actual bug is described in comment 1 :)
Comment 7 Mike Kaganski 2022-10-13 11:17:02 UTC
(In reply to Lionel Elie Mamane from comment #5)
> Your common position makes sense in the absolute, but it is an incompatible
> change that breaks user code :-(

Unfortunately, this position disallows any bugfixing in Basic. The code that accessed missing parameters is wrong, and should be fixed.
Comment 8 Lionel Elie Mamane 2022-10-13 11:59:27 UTC
I've at least documented this in the Release Notes
Comment 9 Mike Kaganski 2022-10-13 12:04:50 UTC
(In reply to Lionel Elie Mamane from comment #8)

Thank you!
But I'm afraid it's not "missing optional procedure arguments would be coerced to a default type value in expressions", but "expressions treated missing parameter as number 448", which is a nonsense. As you note yourself: "if/then construct does coerce a missing parameter to true", and for boolean, a default is false.
Comment 10 Lionel Elie Mamane 2022-10-13 12:12:41 UTC
(In reply to Mike Kaganski from comment #9)
> (In reply to Lionel Elie Mamane from comment #8)
> 
> Thank you!
> But I'm afraid it's not "missing optional procedure arguments would be
> coerced to a default type value in expressions", but "expressions treated
> missing parameter as number 448", which is a nonsense. As you note yourself:
> "if/then construct does coerce a missing parameter to true", and for
> boolean, a default is false.

The release notes are a wiki page. Write what is correct. Since 7.4 is out, I added in both the 7.3 and the 7.4 release notes. Thanks.

https://wiki.documentfoundation.org/ReleaseNotes/7.3
https://wiki.documentfoundation.org/ReleaseNotes/7.4
Comment 11 Mike Kaganski 2022-10-13 13:09:44 UTC
(In reply to Lionel Elie Mamane from comment #10)
> Since 7.4 is out, I added in both the 7.3 and the 7.4 release notes.

Please remove from 7.4 notes, because it's for 7.3. I changed 7.3. notes.
Comment 12 Lionel Elie Mamane 2022-10-13 13:16:45 UTC
(In reply to Mike Kaganski from comment #11)
> (In reply to Lionel Elie Mamane from comment #10)
> > Since 7.4 is out, I added in both the 7.3 and the 7.4 release notes.
> 
> Please remove from 7.4 notes, because it's for 7.3. I changed 7.3. notes.

My reasoning to put it in 7.4 notes was that someone upgrading from 7.3 to 7.4 will read the 7.4 notes, and will not read the changes made to 7.3 notes since he upgraded to 7.3.
Comment 13 Xisco Faulí 2022-10-13 13:19:58 UTC
(In reply to Mike Kaganski from comment #11)
> (In reply to Lionel Elie Mamane from comment #10)
> > Since 7.4 is out, I added in both the 7.3 and the 7.4 release notes.
> 
> Please remove from 7.4 notes, because it's for 7.3. I changed 7.3. notes.

+1
Comment 14 Commit Notification 2022-10-14 08:07:43 UTC
Andreas Heinisch committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/bdfcad586d7b1d4d26bcba50058af7fcb279558d

tdf#151503 - Do not evaluate a missing optional variable to a boolean

It will be available in 7.5.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 15 Commit Notification 2022-10-19 06:34:14 UTC
Andreas Heinisch committed a patch related to this issue.
It has been pushed to "libreoffice-7-4":

https://git.libreoffice.org/core/commit/cc76bafdd1b8b4d03f549785b368e80b9c409c86

tdf#151503 - Do not evaluate a missing optional variable to a boolean

It will be available in 7.4.3.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.