Skip to content

preview/ast-grep #65

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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions .coderabbit.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json
early_access: true
reviews:
path_instructions:
- path: "**/*.{js,ts}"
instructions: "Ensure that all constant variables are defined using UPPER CASE letters"
tools:
ast-grep:
rule_dirs:
- "rules"
- "test-missing-dir"
packages:
- "test-peter-rabbit/test-ast-grep-custom-package"
104 changes: 104 additions & 0 deletions greet.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
/**
* Prints a greeting to the console.
* @param {string} name
* @param {string} surname
*/
export function greet(name, surname) {
console.log(`Hello, ${name} ${surname}!`)
}

/**
* Prints a welcome message to the console.
* @param {string} name
* @param {string} surname
*/
export function welcome(name, surname) {
console.log(`Welcome, ${name} ${surname}!`)
}
Comment on lines +15 to +17
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Console.log usage violates linting rules.

The console.log statement violates the AST-grep linting rule that disallows console.log, console.debug, and console.warn except for console.error inside catch clauses.

Consider replacing with console.error if this is for error logging, or remove the console statement if it's for debugging purposes:

-	console.log(`Welcome, ${name} ${surname}!`)
+	console.error(`Welcome, ${name} ${surname}!`)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export function welcome(name, surname) {
console.log(`Welcome, ${name} ${surname}!`)
}
export function welcome(name, surname) {
console.error(`Welcome, ${name} ${surname}!`)
}
🤖 Prompt for AI Agents
In greet.js around lines 15 to 17, the use of console.log violates linting rules
that disallow console.log outside of catch clauses. To fix this, either remove
the console.log statement if it is for debugging or replace it with
console.error only if it is intended for error logging inside a catch block.
Adjust the code accordingly to comply with the linting restrictions.


import React from 'react'
import PropTypes from 'prop-types'

function HelloWorld({
greeting = "hello",
greeted = '"World"',
silent = false,
onMouseOver,
}) {
if (!greeting) {
console.log("No greeting")
return null
}
Comment on lines +28 to +31
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Console.log usage violates linting rules.

The console.log statement violates the AST-grep linting rule that disallows console.log, console.debug, and console.warn except for console.error inside catch clauses.

Consider replacing with console.error:

-		console.log("No greeting")
+		console.error("No greeting")
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (!greeting) {
console.log("No greeting")
return null
}
if (!greeting) {
- console.log("No greeting")
+ console.error("No greeting")
return null
}
🤖 Prompt for AI Agents
In greet.js around lines 28 to 31, the use of console.log violates linting rules
that only allow console.error inside catch clauses. Replace the console.log("No
greeting") statement with console.error("No greeting") to comply with the
linting rules.


// TODO: Don't use random in render
const num = Math.floor(Math.random() * 1e7)
.toString()
.replace(/.d+/gi, "")
Comment on lines +33 to +36
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix regex pattern and address TODO comment.

Two issues need attention:

  1. The regex pattern /.d+/gi is incorrect - it should be /\d+/gi to match digits
  2. The TODO comment highlights a valid concern about using Math.random() in render, which can cause unnecessary re-renders

Apply this diff to fix the regex:

-		.replace(/.d+/gi, "")
+		.replace(/\d+/gi, "")

Would you like me to suggest an alternative approach to generate the visitor number that doesn't use random in render?

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// TODO: Don't use random in render
const num = Math.floor(Math.random() * 1e7)
.toString()
.replace(/.d+/gi, "")
// TODO: Don't use random in render
const num = Math.floor(Math.random() * 1e7)
.toString()
.replace(/\d+/gi, "")
🤖 Prompt for AI Agents
In greet.js around lines 33 to 36, fix the regex pattern by replacing `/.d+/gi`
with `/\d+/gi` to correctly match digits. Additionally, address the TODO by
removing the use of Math.random() inside the render function to prevent
unnecessary re-renders; instead, generate the visitor number outside the render
or use a stable unique identifier approach.


return (
<div
className="HelloWorld"
title={`You are visitor number ${num}`}
onMouseOver={onMouseOver}
>
<strong>
{greeting.slice(0, 1).toUpperCase() + greeting.slice(1).toLowerCase()}
</strong>
{greeting.endsWith(",") ? (
" "
) : (
<span style={{ color: "grey" }}>", "</span>
)}
<em>{greeted}</em>
{silent ? "." : "!"}
</div>
)
}

HelloWorld.propTypes = {
greeting: PropTypes.string,
greeted: PropTypes.string,
silent: PropTypes.bool,
onMouseOver: PropTypes.func,
}

/**
* Fails ast-grep because of console.log used in the function
* @param {string} text
* @param {string} string
* @returns {boolean}
*/
export function findInString(text, string, logFn = console.error) {
logFn("text", text)
return text.includes(string)
}
return text.includes(string)
}
Comment on lines +71 to +76
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix duplicate code causing parse errors.

There are duplicate lines 75-76 that repeat the return statement and closing brace, causing parse errors as flagged by static analysis.

Remove the duplicate lines:

 export function findInString(text, string, logFn = console.error) {
 	logFn("text", text)
 	return text.includes(string)
 }
-	return text.includes(string)
-}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export function findInString(text, string, logFn = console.error) {
logFn("text", text)
return text.includes(string)
}
return text.includes(string)
}
export function findInString(text, string, logFn = console.error) {
logFn("text", text)
return text.includes(string)
}
🧰 Tools
🪛 Biome (1.9.4)

[error] 75-75: Illegal return statement outside of a function

(parse)


[error] 75-76: Expected a statement but instead found '}'.

Expected a statement here.

(parse)

🤖 Prompt for AI Agents
In greet.js around lines 71 to 76, there is a duplicate return statement and
closing brace causing parse errors. Remove the repeated lines at 75 and 76 so
that the function has only one return statement and one closing brace to fix the
syntax error.


/**
* Fails ast-grep because of console.log used in the catch block
* @param {string} text
* @param {string} string
* @returns {boolean}
*/
export async function findInStringAsync(text, string) {
try {
return text.includes(string)
} catch (error) {
console.log("error", error)
}
}
Comment on lines +84 to +90
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Console.log in catch block violates linting rules.

The console.log statement in the catch block violates the AST-grep linting rule. Only console.error is allowed in catch blocks.

Replace with console.error:

 	} catch (error) {
-		console.log("error", error)
+		console.error("error", error)
 	}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export async function findInStringAsync(text, string) {
try {
return text.includes(string)
} catch (error) {
console.log("error", error)
}
}
export async function findInStringAsync(text, string) {
try {
return text.includes(string)
} catch (error) {
console.error("error", error)
}
}
🤖 Prompt for AI Agents
In greet.js around lines 84 to 90, replace the console.log statement in the
catch block with console.error to comply with linting rules that only allow
console.error in catch blocks. This means changing console.log("error", error)
to console.error("error", error).


/**
* Doesn't fail ast-grep since console.error is allowed in catch block
* @param {string} text
* @param {string} string
* @returns {boolean}
*/
export async function findInStringTreated(text, string) {
try {
return text.includes(string)
} catch (error) {
console.error("error", error)
}
}
22 changes: 22 additions & 0 deletions jsx_ast_grep_failure.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { useState } from 'react'

const greeting: string = "Hello, world!";
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Constant variable name violates coding guidelines.

According to the coding guidelines, all constant variables in JS/TS files must be defined using UPPER CASE letters.

-const greeting: string = "Hello, world!";
+const GREETING: string = "Hello, world!";
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const greeting: string = "Hello, world!";
const GREETING: string = "Hello, world!";
🤖 Prompt for AI Agents
In jsx_ast_grep_failure.ts at line 3, the constant variable name 'greeting'
violates the coding guideline requiring constant variables to be in UPPER CASE.
Rename the variable to 'GREETING' to comply with the naming convention.


async function test() {

const [state, setState] = useState<string>("test string")

const [state2, setState2] = useState<string>("test string")
Comment on lines +7 to +9
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Remove unnecessary TypeScript type annotations.

The explicit <string> type parameters are unnecessary since TypeScript can infer the type from the initial value. This violates the replace-unnecessary-type-definition-use-state rule.

-    const [state, setState] = useState<string>("test string")
+    const [state, setState] = useState("test string")

-    const [state2, setState2] = useState<string>("test string")
+    const [state2, setState2] = useState("test string")
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const [state, setState] = useState<string>("test string")
const [state2, setState2] = useState<string>("test string")
const [state, setState] = useState("test string")
const [state2, setState2] = useState("test string")
🤖 Prompt for AI Agents
In jsx_ast_grep_failure.ts around lines 7 to 9, remove the explicit <string>
type annotations from the useState calls because TypeScript can infer the type
from the initial string value. Simply call useState with the initial string
without specifying the generic type parameter to comply with the
replace-unnecessary-type-definition-use-state rule.


const [first, second] = await Promise.all([
await (new Promise(() => {console.log("logging this long task")})),
new Promise(() => {console.log("logging another long task")}),
])
Comment on lines +11 to +14
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix Promise.all usage and console.log violations.

Multiple issues with this code:

  1. console.log usage violates the no-console-except-error rule
  2. Double await on the first promise is incorrect
  3. Promises don't resolve/reject, causing them to hang indefinitely
-    const [first, second] = await Promise.all([
-        await (new Promise(() => {console.log("logging this long task")})),
-        new Promise(() => {console.log("logging another long task")}),
-    ])
+    const [first, second] = await Promise.all([
+        new Promise((resolve) => {
+            // Removed console.log to comply with no-console-except-error rule
+            resolve("first task completed");
+        }),
+        new Promise((resolve) => {
+            // Removed console.log to comply with no-console-except-error rule
+            resolve("second task completed");
+        }),
+    ])
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const [first, second] = await Promise.all([
await (new Promise(() => {console.log("logging this long task")})),
new Promise(() => {console.log("logging another long task")}),
])
const [first, second] = await Promise.all([
new Promise((resolve) => {
// Removed console.log to comply with no-console-except-error rule
resolve("first task completed");
}),
new Promise((resolve) => {
// Removed console.log to comply with no-console-except-error rule
resolve("second task completed");
}),
])
🧰 Tools
🪛 ast-grep (0.38.6)

[hint] 11-11:
Context: await (new Promise(() => {console.log("logging this long task")}))
Suggested replacement: (new Promise(() => {console.log("logging this long task")}))

(no-await-in-promise-all-custom)

🤖 Prompt for AI Agents
In jsx_ast_grep_failure.ts around lines 11 to 14, fix the Promise.all usage by
removing the redundant double await on the first promise and ensure both
promises properly resolve or reject to avoid hanging. Replace console.log calls
with an allowed logging method or remove them to comply with the
no-console-except-error rule.


return {
state,
setState,
first,
second
}
}
14 changes: 14 additions & 0 deletions rules/no-console-except-error.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
id: no-console-except-error
language: javascript
message: 'Do not use console.$METHOD except for console.error in catch clause'
rule:
any:
- pattern: console.error($$$)
not:
inside:
kind: catch_clause
stopBy: end
- pattern: console.$METHOD($$$)
constraints:
METHOD:
regex: 'log|debug|warn'
7 changes: 7 additions & 0 deletions rules/replace-unnecessary-type-definition-use-state.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
id: replace-unnecessary-type-definition-use-state
language: "typescript"
rule:
any:
- pattern: useState<string>($A)
fix:
useState($A)
15 changes: 15 additions & 0 deletions rules/ts-const.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
id: ts-const
language: Typescript
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix language specification capitalization.

The language should be lowercase "typescript" to match ast-grep's language specifications.

-language: Typescript
+language: typescript
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
language: Typescript
language: typescript
🤖 Prompt for AI Agents
In rules/ts-const.yaml at line 2, change the language specification from
"Typescript" to lowercase "typescript" to comply with ast-grep's expected
language format.

rule:
all:
- any:
- pattern: "const $VAR: $TYPE = $VALUE"
- pattern: "const $VAR = $VALUE"
- inside:
kind: program
constraints:
VAR:
regex: '.*[^A-Z_].*'
severity: warning
message: Always capitalize const variables
note: Const variables should be capitalized