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.

Sign up for GitHub

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

Jump to bottom

Rust: AST: add docs with examples for qltest #17432

Merged
merged 23 commits into from
Sep 16, 2024
Merged

Rust: AST: add docs with examples for qltest #17432

merged 23 commits into from
Sep 16, 2024

Conversation

aibaars
Copy link
Contributor

@aibaars aibaars commented Sep 10, 2024

No description provided.

@aibaars aibaars added the no-change-note-required This PR does not need a change note label Sep 10, 2024
Base automatically changed from rust-experiment to main September 11, 2024 15:31
@github-actions github-actions bot added the Rust Pull requests that update Rust code label Sep 11, 2024
@aibaars aibaars force-pushed the aibaars/rust-doc-tests branch 4 times, most recently from 64320b4 to 280b119 Compare September 12, 2024 14:06
redsun82
redsun82 reviewed Sep 12, 2024
View reviewed changes
rust/schema.py
@@ -119,15 +118,31 @@ class Function(Declaration):
body: Expr | child


# Missing,
@rust.doc_test_signature("() -> ()")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, maybe this should be the default, and there should be a pragma to not wrap the doc code in a function (for top-level declarations)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be handy indeed.

redsun82
redsun82 reviewed Sep 12, 2024
View reviewed changes
rust/schema.py Outdated
Comment on lines 140 to 143
let x = variable;
let x = foo::bar;
let y = <T>::foo
let z = <Type as Trait>::foo
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, how does rust-analyzer and our extractor currently fare with undefined symbols like the ones here? Aren't we getting missing things rather than what we want to test? Also, there might be some semicolons missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extractor and rust-analyzer are fine with missing symbols. Things like jump to definition won't work in the IDE, but the code is still recognized as syntactically valid.

Yeah, I should have added those semicolons, but the parser is quite resilient, so I hadn't noticed. I think it would be nice to also generate a sort of coverage test query that checks that all things appear at least once in the examples.

@aibaars aibaars marked this pull request as ready for review September 12, 2024 18:05
@aibaars
Add some more QL docs
e682298
@aibaars
Avoid use of plain 'None' in tests
d73d90d 
For some reason the parser resolves the ambiguity between None as a PathPat or IdentPat
differently on the Action runners vs local machine.
@aibaars aibaars force-pushed the aibaars/rust-doc-tests branch 2 times, most recently from 71d816c to d73d90d Compare September 13, 2024 12:06
redsun82
redsun82 requested changes Sep 13, 2024
View reviewed changes
Copy link
Contributor

@redsun82 redsun82 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Terrific, looks good! I just would really rather not rename TypeRef to Type.

rust/schema.py
@@ -380,8 +380,8 @@ class MatchArm(AstNode):
A match arm. For example:
```
match x {
Some(y) => y,
None => 0,
Option::Some(y) => y,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe another way of avoiding the ambiguity would be to fix the doc_test_signature to (x: Option<i32>) -> (), here and in all other instances of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That could work, although, does the parser really do name resolution? The strange thing is that on Action runners the same Rust code behaves differently than on macos ... I would like to figure out what on earth is causing the problem, but not for this pull request ;-)

rust/schema.py
@@ -63,50 +63,118 @@ class AstNode(Locatable):


@qltest.skip
class Unimplemented(AstNode):
class Unimplemented(Element):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another approach that works (I used it in my draft type PR here) is to keep one Unimplmented, but make it a subclass of every AST kind that can be unimplemented (e.g. Unimplemented(Declaration, TypeRef)). That has the advantage that we can keep one emit_unimplemented method in the translator.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea for a follow-up PR.

rust/schema.py
pass


@qltest.skip
@rust.doc_test_signature("() -> ()")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's get this in, then I can make this the default and clean up

rust/schema.py Outdated Show resolved Hide resolved
rust/schema.py Outdated Show resolved Hide resolved
rust/schema.py Outdated Show resolved Hide resolved
@aibaars
Copy link
Contributor Author

aibaars commented Sep 13, 2024

@redsun82 I agree that BinaryExpr, LiteralExpr and TypeRef would be better names. My idea is to generate the entire extractor from https://github.com/rust-lang/rust-analyzer/blob/master/crates/syntax/rust.ungram which uses those names. I guess the extractor generator could remap those names though. It's only a minor complication.

@redsun82
Copy link
Contributor

@redsun82 I agree that BinaryExpr, LiteralExpr and TypeRef would be better names. My idea is to generate the entire extractor from https://github.com/rust-lang/rust-analyzer/blob/master/crates/syntax/rust.ungram which uses those names. I guess the extractor generator could remap those names though. It's only a minor complication.

Yeah, I think it's a good idea to reuse ungram, but we should make sure the naming and structure there doesn't "pollute" our QL definitions. I think we can achieve that, maybe with some rust annotations on schema.py? There's also the question of extracting the desugared AST: if we model it as a field in AstNode, we will need to fill it in in the generated structures which might be tricky. We could "detach" the field from the generated rust class, but it might either mean using a hard coded included dbscheme relation, or some admittedly minor work on the code generation.

@redsun82 redsun82 merged commit e1ac40e into main Sep 16, 2024
14 checks passed
@redsun82 redsun82 deleted the aibaars/rust-doc-tests branch September 16, 2024 07:00
@paldepind
Copy link
Contributor

Thanks for this. This documentation is super helpful 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@redsun82 redsun82 redsun82 approved these changes

Assignees
No one assigned
Labels
no-change-note-required This PR does not need a change note Rust Pull requests that update Rust code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aibaars @redsun82 @paldepind