Conversation
this includes unimplemented: - definition span annotations - event graph annotations this does not include a public API to obtain the JSON output yet
this invokes and outputs the internally implemented ast-to-JSON conversion
This commit adds experimental support for collecting a set of errors instead of bailing out on any error. These errors are reported within the AST as an example.
| let collect_errors : bool ref = ref false | ||
|
|
||
| (* Method injection *) | ||
| let raise (e: exn) = |
There was a problem hiding this comment.
This is problematic right ?
Sometimes we may have structural errors, after which we should not continue
There was a problem hiding this comment.
That's why there's an alternate pathway where errors can be raised through raise_fatal instead.
The idea is this intercepts all raise methods in the file if opened, and therefore makes the default behaviour fallthrough and we can use the fatal variant if we can't recover from it.
Generally, if there are strict structural errors that we can't handle, the OCaml type checker will prevent compilation anyways, since raise will fall-through with a return type unit, in which case we can decide ourselves if there's a fallback we can use or if we should change it to be a fatal exception.
arj4web
left a comment
There was a problem hiding this comment.
This almost seems fine, except for a few redundant additions and what appear to be some genuine errors.
You can try fixing the errors first.
This PR is very detail-oriented, so hopefully I didn't miss anything major. I'll still ask Copilot to look for any remaining details.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 32 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
lib/graphBuilder.ml:177
- The argument-count check for function calls only guards the
raiseline;List.iter2and the recursive call execute unconditionally. WhenErrorCollector.collect_errorsis enabled,raisewill not abort, soList.iter2can throw due to list-length mismatch and crash instead of producing a collected compiler error.
if List.length td_args <> List.length func.args then
raise (event_graph_error_default "Arguments missing in function call" e.span);
List.iter2 (fun td arg ->
| let cunit_json = compilation_unit_with_event_graph_to_yojson cunit gcol_list in | ||
| let _ = assert (cunit.cunit_file_name = Some fname) in | ||
| cunit_json |
There was a problem hiding this comment.
Do we want to fix this? Is it possible that there's no file name when -ast is supplied?
There was a problem hiding this comment.
Ok I suppose this is an implicit invariant specific to the implementation. assertion looks reasonable
7ccf8d4 to
f9ff1b4
Compare
f9ff1b4 to
03660e1
Compare
|
@wxwern Sorry let me review in the next few days. Always forgot |
| (** Create successful JSON output *) | ||
| let success_output output_str = | ||
| { success = true; errors = []; output = Some output_str } | ||
| let transpiled_output output_str = |
There was a problem hiding this comment.
Perhaps unimportant: why is it called "transpile"?
There was a problem hiding this comment.
cuz it's transpiled to system verilog technically and it makes it clear that this is source code output
| let has_collected_errors () : bool = | ||
| !_global_errors <> [] | ||
|
|
||
| let dedup_collected_errors () = |
There was a problem hiding this comment.
Why do we need to do dedup?
There was a problem hiding this comment.
for non-blocking errors many will be re-reported if a validation pathway reencounters the same issue after a loop or any repeated evaluation, and it was easier to just deduplicate it than to trace every instance of duplication
| add_msg msg ev ({d = {ty = Recv msg; until = ev}; def_span = ac_span.def_span; action_event = ac_span.action_event; span = ac_span.span}) | ||
| | ImmediateSend (msg, td) -> | ||
| add_msg msg ev ({d = {ty = Send (msg, td); until = ev}; span = ac_span.span}) | ||
| add_msg msg ev ({d = {ty = Send (msg, td); until = ev}; def_span = ac_span.def_span; action_event = ac_span.action_event; span = ac_span.span}) |
There was a problem hiding this comment.
This pattern shows up quite a lot it seems. Maybe can have a polymorphic function, something like map : (U -> V) -> (U ast_node) -> (V ast_node).
There was a problem hiding this comment.
this would result in a much larger refactor than the already large PR, but i'm fine if y'all are fine with that
jasonyu1996
left a comment
There was a problem hiding this comment.
Look good overall. Just some minor questions/nits
A preliminary implementation of outputting annotated AST information from the AnvilHDL compiler.
This allows for language servers to query AST information from Anvil, and supply hover hints, jump to definition, etc., to code editors, for an improved user experience.
Together with anvil-lsp, this resolves #73.
Current limitations:
Compiler warnings are lone print statements and have no mechanism to be collected and outputted at this time. This should be added in the future.
Not all symbols are annotated with their exact ranges without much more significant refactors. This makes it impossible to support symbol renames under LSP.
Definitions and types with unknown parameters are not analyzed by the compiler. This means most LSP features currently don't work correctly if a definition contains type parameters.
Important side effects:
ast_nodeor have codespan information added.ErrorCollectorto support collecting multiple errors, by injecting a modifiedraisefunction into the global scope when opened.