Aligning implementation/specification error codes - phase 3 - #1989
Aligning implementation/specification error codes - phase 3#1989Roman-Manevich wants to merge 7 commits into
Conversation
71be2cd to
79d0133
Compare
| "array@ length@ expression@ %a@ has@ negative@ length:@ %i." pp_expr | ||
| e_length length | ||
| | MultipleWrites id -> pp_err Parse "multiple@ writes@ to@ %S." id | ||
| | MultipleWrites id -> pp_err Typing "multiple@ writes@ to@ %S." id |
There was a problem hiding this comment.
This error is reported during AST building (desugaring specifically) - why is it a typing error?
There was a problem hiding this comment.
This is a bit subtle. AssignableExpressions.tex defines the (AST) desugaring rule by calling check_no_duplicates, which returns a type error. We can roll back to Parse but then there's a discrepancy between the implementation and documentation.
Alternatively, we could change the spec (say, by creating a dedicated duplicates check) so that raises a build error.
There was a problem hiding this comment.
Reverted Typing back to Parse.
Changed the spec to compare the cardinality of the set of fields with the length of the list and have it emit BE_PE if they differ.
| | BadArity of error_handling_time * identifier * int * int | ||
| | BadCallArity of { name : identifier; expected : int; provided : int } | ||
| | BadTupleArity of { expected : int; actual : int } | ||
| | BadParameterArity of error_handling_time * version * identifier * int * int |
There was a problem hiding this comment.
What are the status of BadArity and BadParameterArity after the new Bad{Call,Tuple}Arity? For example, is BadArity an V0-only error, and only used with Dynamic? If so, we should remove error_handling_time from BadArity.
There was a problem hiding this comment.
BadArity is used in both ASLv0 and ASLv1 contexts and its error_handling_time depends on the configuration passed to Make in Interpreter.ml so both static/dynamic are possible. The situation is similar with BadParameterArity.
There was a problem hiding this comment.
Uses of BadArity seem to be:
- In
Interpreter.mlfor:- incorrect argument/parameter arity for a subprogram call
protected_multi_assignrun_typed
- in
Native.mlfor primitives
From a brief look, these all seem to be for untyped specifications - I believe ASL1 type-checking would catch all of these before interpretation. Am I missing something?
There was a problem hiding this comment.
answered offline.
| fatal_from ~loc | ||
| @@ Error.BadArity | ||
| (Static, name, List.length callee.args, List.length args1) | ||
| @@ Error.BadCallArity |
There was a problem hiding this comment.
This is an ASL0-only error site - does it need to change?
There was a problem hiding this comment.
As long as we support ASL0, this seems like an improvement in terms of the error code and diagnostic message.
There was a problem hiding this comment.
ASL0 does not have error codes
There was a problem hiding this comment.
Still, this is consistent with the same check for ASLv1. I don't see how this hurts.
79d0133 to
0077e04
Compare
| else | ||
| fatal_from e1 env (Error.MismatchType (B.debug_value v, [ t.desc ]))) | ||
| fatal_from e1 env | ||
| (Error.ATCFailure (C.error_handling_time, B.debug_value v, t.desc))) |
There was a problem hiding this comment.
Maybe we should have something a bit more flexible than debug_value, ideally a formatter
Probably not for this PR.
| @@ -42,6 +42,8 @@ type error_desc = | |||
| found_call_type : subprogram_type; | |||
| } | |||
| | BadArity of error_handling_time * identifier * int * int | |||
There was a problem hiding this comment.
What is BadArity used for then? Maybe add a little comment to explain this?
There was a problem hiding this comment.
How about
An arity mismatch detected during evaluation: a subprogram or
primitive receives the wrong number of arguments, a tuple assignment
receives the wrong number of values from a subprogram call, or an
entry point returns the wrong number of values.
?
There was a problem hiding this comment.
I like it, what about adding quickly at the start that this should not happen if type-checking worked correctly?
|
|
||
|
|
||
| ASL Lexical error (BE_LE): Unknown symbol. | ||
| ASL Lexical error (BE_LE): Unknown symbol "". |
There was a problem hiding this comment.
Oh that's ugly, should we create an issue about this?
0077e04 to
3ca79fc
Compare
3ca79fc to
3c1daa3
Compare
HadrienRenaud
left a comment
There was a problem hiding this comment.
This looks ok to me, but please double check with Hrutvik that he's happy with it
| | BadCallArity of { name : identifier; expected : int; provided : int } | ||
| | BadTupleArity of { expected : int; actual : int } |
There was a problem hiding this comment.
There's a naming mismatch between those 2 declaration: should we use provided or actual?
| (** An arity mismatch detected during evaluation: a subprogram or | ||
| primitive receives the wrong number of arguments, a tuple assignment | ||
| receives the wrong number of values from a subprogram call, or an | ||
| entry point returns the wrong number of values. *) |
There was a problem hiding this comment.
This is nice, just a tiny edit:
| (** An arity mismatch detected during evaluation: a subprogram or | |
| primitive receives the wrong number of arguments, a tuple assignment | |
| receives the wrong number of values from a subprogram call, or an | |
| entry point returns the wrong number of values. *) | |
| (** [BadArity (time, name, expected, provided)] is raised when there is | |
| an arity mismatch detected during evaluation when it should have been | |
| detected at type-checking: a subprogram or | |
| primitive receives the wrong number of arguments, a tuple assignment | |
| receives the wrong number of values from a subprogram call, or an | |
| entry point returns the wrong number of values. *) |
Distinguish lexical and parse failures
Lexer-detected invalid symbols now use
UnknownSymboland reportBE_LE, preserving suggested replacements, while the remainingCannotParseerrors reportBE_PE.Classify global-initialization exceptions
Exceptions escaping global initialization now report
DE_UE, andbuild_genvinasl.specconverts the corresponding throwing result toDynamicError(DE_UE).Classify asserted-type-conversion failures
Failed asserted type conversions now use
ATCFailure, reportingDE_TAFduring dynamic evaluation andTE_SEFduring static evaluation as specified byasl.spec.Classify repeated tuple-field writes
Repeated fields in tuple assignments now report
TE_IAD, matching the specification's use ofcheck_no_duplicatesrather than the rework PR's disputedBE_PEmapping.Distinguish call and tuple arity failures
Static call arity failures now report
TE_BCthroughBadCallArity, while tuple arity failures reportTE_UTthroughBadTupleArity; unchecked runtime arity failures remain uncoded.