Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

The parser is overly permissive of values types #10

Open
Bike opened this issue Jun 16, 2021 · 4 comments
Open

The parser is overly permissive of values types #10

Bike opened this issue Jun 16, 2021 · 4 comments
Labels
bug Something isn't working

Comments

@Bike
Copy link
Member

Bike commented Jun 16, 2021

For example, (ctype:specifier-ctype '(function ((values integer)))) returns a ctype with a values type in the lambda list, which is nonsensical. It should signal a syntax error.

It would probably be good to have separate specifier-ctype and values-specifier-ctype functions, where the former errs on values types and the latter coerces everything to a values type, and then call them appropriately. But I'm not sure how that would work with the customization, i.e. cons-specifier-ctype being generic.

@Bike Bike added the bug Something isn't working label Jun 16, 2021
@digikar99
Copy link
Contributor

I could imagine there being another parameter values to cons-specifier-ctype that is either of type null or t.

But I don't feel having a separate specifier-ctype and values-specifier-ctype is a good way forward, or I don't get the motivation for doing this. I am unable to see what goes wrong/difficult with having just specifier-ctype and do the appropriate checks sticking as close to CLHS/implementations as possible, disallowing types like (function ((values integer))). (May be stick to sb-kernel:specifier-type unless it becomes unreasonable?)

@Bike
Copy link
Member Author

Bike commented Jun 17, 2021

specifier-ctype and values-specifier-ctype would be how those checks are done. For example the function type parser would call specifier-ctype for the parameter types, which would signal an error if a values type is written there, and then would call values-specifier-ctype for the return value type, which would accept values or non-values but coerce the latter.

I'm not sure I understand what you mean. This would be fixing a CLHS incompatibility in ctype, and it is roughly what SBCL does in its own system. You can see here - arg types are parsed with sb-kernel:specifier-type (https://github.com/sbcl/sbcl/blob/ab6658fec390036865dabb4c77722f45d48e7700/src/code/type.lisp#L622) whereas the result type is parsed with basic-parse-typespec and coerce-to-values (https://github.com/sbcl/sbcl/blob/ab6658fec390036865dabb4c77722f45d48e7700/src/code/type.lisp#L680), in a way similar to the sb-kernel:values-specifier-type function here https://github.com/sbcl/sbcl/blob/ab6658fec390036865dabb4c77722f45d48e7700/src/code/type.lisp#L1582-L1589

eta: the way sb-kernel:specifier-type works is it just errors if the underlying parser returns a values type. that's probably doable easy

@digikar99
Copy link
Contributor

Okay, TIL about sb-kernel:values-specifier-type, and yes given the following behavior, things make sense:

CL-USER> (sb-kernel:values-specifier-type '(values &rest t))
#<SB-KERNEL:NAMED-TYPE *>
CL-USER> (sb-kernel:specifier-type '(values &rest t))
; Evaluation aborted on #<SIMPLE-ERROR #<SB-FORMAT::FMT-CONTROL "VALUES type illegal in this context:~% ~/SB-IMPL:PRINT-TYPE-SPECIFIER/"> {102D3A3963}>.

If the actual work is being done by sb-kernel::basic-parse-typespec, then sb-kernel:specifier-type and ctype:specifier-ctype are only needed so long as we want to provide the guarantee that the return value is a non-values type. My previous understanding of specifier-ctype (and current semantics in the code?) was that it was meant as the entry point into the parser. Instead that role could be subsumed by something equivalent to basic-parse-typespec - so basic-parse-typespec could serve as the entry point into the parser, while specifier-ctype and values-specifier-ctype could serve as wrappers around it; in essence, the current specifier-ctype could be renamed to basic-parse-typespec or equivalent, and a new specifier-ctype that ensures that its return value is not cvalues could be defined. But I don't see the need to touch cons-specifier-ctype in this way of doing things.

One other alternative to implementing cons-specifier-ctype itself as a generic function could be to keep cons-specifier-ctype as a normal non-generic function, and instead of using etypecase inside it, use typecase, and make the otherwise case dispatch on a generic-function say nonstandard-cons-specifier-ctype. (Don't recall ATM if there were any downsides to this way, or I just overlooked this solution back then.)

@Bike
Copy link
Member Author

Bike commented Jun 18, 2021

yeah, shouldn't need to change cons-specifier-ctype if it works like sbcl

Bike added a commit that referenced this issue Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants