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

feat(diagnostics): unused expression result detector #622

Merged
merged 19 commits into from
Jan 17, 2024
Merged

feat(diagnostics): unused expression result detector #622

merged 19 commits into from
Jan 17, 2024

Conversation

pront
Copy link
Member

@pront pront commented Jan 4, 2024 •
edited
Loading

https://datadoghq.atlassian.net/browse/OPW-92

Summary

  • After compilation, do another AST traversal
  • Discover unused expression results
    • including unused variables (shadowing not supported)
  • Produce warnings including source code spans for the unused expression
  • The goal is to catch as much unused expressions as possible but not all of them. We do have to eliminate all false positives though.

Testing plan

  • Unit tests
  • Tweak the testing harness to compare emitted diagnostics
  • Fix all existing VRL tests (warnings are treated as errors by the test harness)

@pront pront force-pushed the pront/unused-warning branch 5 times, most recently from 3919db5 to 25ebe98 Compare January 5, 2024 15:11
@pront pront changed the title WIP feat(diagnostics): unused expression result detector Jan 5, 2024
@pront pront force-pushed the pront/unused-warning branch 4 times, most recently from df95222 to bbaa68c Compare January 5, 2024 16:24
@pront pront requested review from fuchsnj and removed request for fuchsnj January 5, 2024 18:59
@pront pront force-pushed the pront/unused-warning branch from 6a258b3 to d865972 Compare January 8, 2024 20:24
@pront pront marked this pull request as ready for review January 8, 2024 20:24
pront
pront commented Jan 8, 2024
View reviewed changes
src/compiler/unused_expression_checker.rs Outdated
impl AstVisitor<'_> {
fn visit_node(&self, node: &Node<Expr>, state: &mut VisitorState) {
let expression = node.inner();
// println!("\n{} visit_node: {expression:#?}", state.level);
Copy link
Member Author

Choose a reason for hiding this comment

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

Reminder: All // println!(... lines should be be deleted before merging.

fuchsnj
fuchsnj approved these changes Jan 17, 2024
View reviewed changes
Copy link
Member

@fuchsnj fuchsnj left a comment

Choose a reason for hiding this comment

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

This looks reasonable from a high level. I'm glad the unused_expression_checker was kept separate from the main compiler, that should hopefully keep it more maintainable.

@pront pront added this pull request to the merge queue Jan 17, 2024
Merged via the queue into main with commit b30a594 Jan 17, 2024
10 checks passed
@pront pront deleted the pront/unused-warning branch January 17, 2024 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fuchsnj fuchsnj fuchsnj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pront @fuchsnj