topdown: Make *.is_valid function behavior more consistent

[philipaconrad]

This PR changes the behavior of the *.is_valid builtins to consistently return only true/false values, with no runtime/builtin errors. No docs changes should be required, since this is the documented behavior of these functions.

The affected list of builtins:

Encoding:

• base64.is_valid
• json.is_valid
• yaml.is_valid

GraphQL:

• graphql.is_valid

Regex:

• regex.is_valid (already implements behavior)

Semver:

• semver.is_valid

Questionable additions to the list:

• io.jwt.verifyX functions?

Fixes #4760.

[ashutosh-narkar]

This looks good. Any tests need to be added/updated. Also does this affect strict-builtin-errors behavior?

[philipaconrad]

Any tests need to be added/updated. Also does this affect strict-builtin-errors behavior?

I'll update the affected tests soon.

This does not change strict-builtin-errors behavior overall, but does mean that the builtins in the affected list above will no longer produce builtin errors / undefined when they hit an error condition, such as a bad argument. Instead, they will just return false.

[anderseknert]

Thanks for picking this up!

perhaps it would be good to have a test with strict built in errors set to true, and assert the same behavior as when set to false?

[philipaconrad]

perhaps it would be good to have a test with strict built in errors set to true, and assert the same behavior as when set to false?

--strict-builtin-errors causes actual errors though for the test cases that are changed here. 🤔 My understanding of the original issue was to replace the builtin error cases for the affected is_valid functions with return false cases.

Are there some types of errors we want to keep, even if they affect the true/false outputs?

[anderseknert]

@philipaconrad OK! How does that work, given the functions return boolean and not errors now? If I provide e.g. an int to semver.is_valid, is that false without strict mode and an error with it on?

[srenatus]

If I provide e.g. an int to semver.is_valid, is that false without strict mode and an error with it on?

It'll be false, regardless of strict-builtin-errors.

[anderseknert]

It'll be false, regardless of strict-builtin-errors.

Thanks @srenatus. This is what I wanted a test to assert.

[philipaconrad]

Changes squashed, should be ready-for-merge.

The old test cases that checked for builtin errors on the affected *.is_valid builtins now check for the value false, so I think that should cover most of what @anderseknert requested in the way of test cases.

[anderseknert]

@philipaconrad not terribly important, but what I wanted was a test that kept the strict_error attribute set to true, and asserted that the value returned is still false.

[philipaconrad]

@philipaconrad not terribly important, but what I wanted was a test that kept the strict_error attribute set to true, and asserted that the value returned is still false.

@anderseknert Done. I took the old json.is_valid tests, and added the strict_error attribute on each of them, which I think should do what you were asking for.