add Compilation API
#1194
add Compilation API
#1194
Conversation
🦋 Changeset detectedLatest commit: 8d72d5d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| BindingGraph, BindingLocation, BuiltInLocation, UserFileLocation, | ||
| }; | ||
|
|
||
| /// TODO: This is a work-around for the fact that `metaslang_bindings` internals (handles, locators, etc...) are exposed. |
There was a problem hiding this comment.
@ggiraldez these are the wrappers I mentioned, due to the fact that the original Definition and Reference types have handles in them.
They should be removed once the metaslang_bindings API is fixed.
There was a problem hiding this comment.
Is the problem we don't want to expose those handles to Typescript? The handles themselves are usize values. Or is the pointer/reference to the bindings object that's the problem? I recall you mentioning that they should be copyable.
Since we're going to pre-resolve all references anyway, we may just get away with cursors in the end. But asking to understand and to be aware of what are the limitations.
There was a problem hiding this comment.
is the pointer/reference to the bindings object that's the problem?
Yes. For WIT resources, we need to track their lifetimes on the other side of the FFI boundary, and there isn't (currently) a straightforward way to do that with the handle references.
For the existing WIT resources, we either wrap them in Rc<> (define_rc_wrapper()) or RefCell<> (define_refcell_wrapper()).
| @@ -267,7 +271,7 @@ impl<KT: KindTypes + 'static> Bindings<KT> { | |||
|
|
|||
There was a problem hiding this comment.
@ggiraldez unrelated to the PR, but I wonder if this TODO is already fixed?
// TODO: what should we do if the parent reference
// cannot be resolved at this point?
There was a problem hiding this comment.
It's not, but I don't know if there's something meaningful we can do in this case. For now, if we cannot resolve a parent it's simply ignored.
This resolution method is used internally, while resolving other references. Eg. for determining which methods are reachable when resolving a virtual call; or to determine which extension scopes (ie. attached functions) apply in a particular context. Resolving parents in these circumstances should not be a problem unless the user code is invalid, or if there's a bug in the binding rules.
There was a problem hiding this comment.
unless the user code is invalid
Which is a possible use case, right?
if we cannot resolve a parent it's simply ignored
Sounds good. I will use None in this case.
There was a problem hiding this comment.
Yes, it's certainly possible that the user code is invalid and that means we cannot resolve a parent reference. Eg. definining contract Test is Base {} without defining Base. This failure to resolve is currently ignored. I think my main concern for writing the TODO was that while this may impact resolved references, there's no feedback given to the user indicating that this may be a problem. In general, the current behaviour should be ok though.
|
|
||
| const defs = reference.definitions(); | ||
| assert.equal(defs.length, 0); | ||
| // TODO: there should be one built-in definition, right? |
There was a problem hiding this comment.
@ggiraldez I'm getting this result, and I don't think it is correct.
IIUC, The commented out assertion should have succeeded, right?
There was a problem hiding this comment.
Correct. This is probably because there's a missing step when constructing the BindingsGraph in CompilationUnit to add the built-ins. Since this is language dependant, I don't think it's possible to do in the runtime. As of today's base main this is also a bit cumbersome, but I streamlined the process in #1170. It may be worth it to wait for it to be merged (shouldn't be too far ahead) to find a good solution for this.
There was a problem hiding this comment.
I made the refactoring, and added a couple of TODOs that you might want to check for the remaining work. I didn't want to do a larger refactoring of bindings until you get a chance to review/rebase on top.
OmarTawfik
force-pushed
the
compilation-api
branch
from
4003a6c to
950d218
Compare
| }; | ||
|
|
||
| let mut binding_graph = | ||
| create_with_resolver(self.language_version.clone(), Rc::new(resolver)); |
There was a problem hiding this comment.
As mentioned before, there's a missing step here, adding the built-ins to the bindings graph. There are some changes in #1170 to streamline this process, but it's still language dependant (ie. currently only available for Solidity), so it's not possible to introduce that code here. I'm not sure what's a good solution for this problem.
There was a problem hiding this comment.
Good catch. Will move to extensions.
| @@ -0,0 +1,47 @@ | |||
| // This file is generated automatically by infrastructure scripts. Please don't edit by hand. | |||
There was a problem hiding this comment.
This file is not automatically generated IIUC. Is this a leftover?
| @@ -0,0 +1,213 @@ | |||
| // This file is generated automatically by infrastructure scripts. Please don't edit by hand. | |||
There was a problem hiding this comment.
Removed from all files in this directory.
| path: [StringLiteral | ||
| @variant ([DoubleQuotedStringLiteral] | [SingleQuotedStringLiteral]) | ||
| ] |
There was a problem hiding this comment.
Why are we capturing the string literal variant instead of the StringLiteral? I suspect it's to return terminal nodes, but since we're using cursors it shouldn't make a big difference and I expect users to .unparse() the cursor anyways. Am I missing something?
There was a problem hiding this comment.
The difference is that unparsing a non-terminal can also return the trivia around it, so it will be confusing for users to deal with leading whitespace in ( "foo.sol").
There was a problem hiding this comment.
Ah! I see. Thanks for the explanation.
| SourceFileNotFound(String), | ||
| } | ||
|
|
||
| fn extract_import_paths(cursor: Cursor) -> Result<Vec<Cursor>, String> { |
There was a problem hiding this comment.
This function's implementation is Solidity specific. Should we extract a SPI and move it to the slang_solidity crate instead of the generator? This is a similar conundrum we have with built-ins and I'd be interested a better solution for them as well.
| use crate::cst::{Cursor, Query}; | ||
| use crate::parser::{Parser, ParserInitializationError}; | ||
|
|
||
| pub struct InternalCompilationBuilder { |
There was a problem hiding this comment.
Should this be called CompilationBuilder? I assume the Internal prefix is because it's only used for the Typescript interface, but looks like a useful component for Rust users as well.
There was a problem hiding this comment.
After reading the whole PR I got to the actual CompilationBuilder. I still find the Internal prefix a bit confusing here, but it's more a nit than anything else.
There was a problem hiding this comment.
I was thinking that we would then expose a similar callback-based API in Rust as well using a new CompilationBuilder that would wrap the InternalCompilationBuilder here. WDYT?
I'm happy to rename it as well if you have a better idea.
| .map_err(AddFileError::Internal)?; | ||
|
|
||
| let file = File::new(id.clone(), parse_output.tree().clone()); | ||
| self.files.insert(id, file); |
There was a problem hiding this comment.
Should we check if the file was already added?
There was a problem hiding this comment.
Sure. We do that check in TS, but wouldn't hurt to add it here as well.
| let built_ins_cursor = transform_built_ins_node(&built_ins_parse_output.tree()) | ||
| .cursor_with_offset(TextIndex::ZERO); | ||
| let built_ins_cursor = | ||
| transform_built_ins_node(built_ins_parse_output.tree()).cursor_with_offset(TextIndex::ZERO); | ||
|
|
||
| bindings.add_system_file("built_ins.sol", built_ins_cursor); | ||
| Ok(bindings) | ||
| binding_graph.add_system_file("built_ins.sol", built_ins_cursor); | ||
| Ok(binding_graph) |
There was a problem hiding this comment.
This is the part that's missing for the Typescript interface or InternalCompilationBuilder. It's simplified in #1170 to just
add_built_ins(&mut bindings, language_version)?;
There was a problem hiding this comment.
Will move it to the extensions module.
| Some(path_to_resolve.to_string()) | ||
| impl PathResolver<KindTypes> for NoOpResolver { | ||
| fn resolve_path(&self, _context_path: &str, path_to_resolve: &Cursor) -> Option<String> { | ||
| Some(path_to_resolve.node().unparse()) |
There was a problem hiding this comment.
I don't think this is important because the files being tested are unrelated, but for correctness this should remove the quotes surrounding the string literal received.
And along, a few fixes to the parser and binding APIs (more details in the added changesets).
OmarTawfik
force-pushed
the
compilation-api
branch
from
950d218 to
1b39fb2
Compare
| @@ -4,7 +4,7 @@ codegen_language_macros::compile!(Language( | |||
| name = Solidity, | |||
| documentation_dir = "crates/solidity/inputs/language/docs", | |||
| binding_rules_file = "crates/solidity/inputs/language/bindings/rules.msgb", | |||
| file_extension = ".sol", | |||
| file_extension = ".sol", // TODO: This should be moved to the Solidity-specific 'extensions' sub-module. | |||
There was a problem hiding this comment.
I was thinking it may make sense to put this in the new LanguageFacts struct. WDYT?
There was a problem hiding this comment.
mmm, What is the use case to expose this in the public API?
And along, a few fixes to the parser and binding APIs (more details in the added changesets).