Skip to content

Commit 571f0de

Browse files
guillaume-dequennesonartech
guillaume-dequenne
authored and
sonartech
committed
SONARPY-2977 Add quickfix to S7489: Async functions should not contain synchronous OS calls (#295)
GitOrigin-RevId: 24ee4bbb1f25604932b2bef09710253dc1eca529
1 parent e7cce5d commit 571f0de

4 files changed

Lines changed: 292 additions & 1 deletion

File tree

‎python-checks/src/main/java/org/sonar/python/checks/SynchronousOsCallsInAsyncCheck.java‎

Lines changed: 68 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,19 +16,34 @@
1616
*/
1717
package org.sonar.python.checks;
1818

19+
import java.util.HashMap;
20+
import java.util.List;
21+
import java.util.Map;
22+
import java.util.Optional;
1923
import java.util.Set;
2024
import org.sonar.check.Rule;
2125
import org.sonar.plugins.python.api.PythonSubscriptionCheck;
2226
import org.sonar.plugins.python.api.SubscriptionContext;
27+
import org.sonar.plugins.python.api.quickfix.PythonQuickFix;
28+
import org.sonar.plugins.python.api.tree.AliasedName;
2329
import org.sonar.plugins.python.api.tree.CallExpression;
30+
import org.sonar.plugins.python.api.tree.ImportName;
31+
import org.sonar.plugins.python.api.tree.Name;
2432
import org.sonar.plugins.python.api.tree.Tree;
33+
import org.sonar.python.quickfix.TextEditUtils;
2534
import org.sonar.python.tree.TreeUtils;
2635
import org.sonar.python.types.v2.TypeCheckMap;
2736

2837
@Rule(key = "S7489")
2938
public class SynchronousOsCallsInAsyncCheck extends PythonSubscriptionCheck {
3039
private static final String MESSAGE = "Use a thread executor to wrap blocking OS calls in this async function.";
3140
private static final String SECONDARY_MESSAGE = "This function is async.";
41+
private static final String TRIO_QUICK_FIX = "Wrap with \"await trio.thread.executor\".";
42+
private static final String ANYIO_QUICK_FIX = "Wrap with \"await anyio.thread.executor\".";
43+
44+
private static final String TRIO = "trio";
45+
private static final String ANYIO = "anyio";
46+
private static final String THREAD_RUN_SYNC_FORMAT = "%s.to_thread.run_sync";
3247

3348
private static final Set<String> OS_BLOCKING_CALLS = Set.of(
3449
"os.wait",
@@ -37,10 +52,12 @@ public class SynchronousOsCallsInAsyncCheck extends PythonSubscriptionCheck {
3752
);
3853

3954
private TypeCheckMap<Object> syncOsCallsTypeChecks;
55+
private final Map<String, String> asyncLibraryAliases = new HashMap<>();
4056

4157
@Override
4258
public void initialize(Context context) {
4359
context.registerSyntaxNodeConsumer(Tree.Kind.FILE_INPUT, this::setupTypeChecks);
60+
context.registerSyntaxNodeConsumer(Tree.Kind.IMPORT_NAME, this::trackAsyncLibraryImports);
4461
context.registerSyntaxNodeConsumer(Tree.Kind.CALL_EXPR, this::checkSyncOsCallsInAsync);
4562
}
4663

@@ -49,6 +66,23 @@ private void setupTypeChecks(SubscriptionContext ctx) {
4966
var object = new Object();
5067
OS_BLOCKING_CALLS.forEach(path ->
5168
syncOsCallsTypeChecks.put(ctx.typeChecker().typeCheckBuilder().isTypeOrInstanceWithName(path), object));
69+
asyncLibraryAliases.clear();
70+
}
71+
72+
private void trackAsyncLibraryImports(SubscriptionContext ctx) {
73+
ImportName importName = (ImportName) ctx.syntaxNode();
74+
for (AliasedName module : importName.modules()) {
75+
List<Name> names = module.dottedName().names();
76+
if (names.size() > 1) {
77+
continue;
78+
}
79+
String moduleName = names.get(0).name();
80+
Name alias = module.alias();
81+
String moduleAlias = alias != null ? alias.name() : moduleName;
82+
if (TRIO.equals(moduleName) || ANYIO.equals(moduleName)) {
83+
asyncLibraryAliases.put(moduleName, moduleAlias);
84+
}
85+
}
5286
}
5387

5488
private void checkSyncOsCallsInAsync(SubscriptionContext ctx) {
@@ -59,6 +93,39 @@ private void checkSyncOsCallsInAsync(SubscriptionContext ctx) {
5993
}
6094

6195
syncOsCallsTypeChecks.getOptionalForType(callExpression.callee().typeV2())
62-
.ifPresent(object -> ctx.addIssue(callExpression.callee(), MESSAGE).secondary(asyncToken, SECONDARY_MESSAGE));
96+
.ifPresent(object -> {
97+
var issue = ctx.addIssue(callExpression.callee(), MESSAGE).secondary(asyncToken, SECONDARY_MESSAGE);
98+
addQuickFixes(issue, callExpression);
99+
});
100+
}
101+
102+
private void addQuickFixes(PreciseIssue issue, CallExpression callExpression) {
103+
Optional<String> calleeDottedName = TreeUtils.stringValueFromNameOrQualifiedExpression(callExpression.callee());
104+
105+
if (calleeDottedName.isPresent()) {
106+
if (asyncLibraryAliases.containsKey(TRIO)) {
107+
String alias = asyncLibraryAliases.get(TRIO);
108+
issue.addQuickFix(createThreadExecutorQuickFix(callExpression, String.format(THREAD_RUN_SYNC_FORMAT, alias),
109+
calleeDottedName.get(), TRIO_QUICK_FIX));
110+
}
111+
112+
if (asyncLibraryAliases.containsKey(ANYIO)) {
113+
String alias = asyncLibraryAliases.get(ANYIO);
114+
issue.addQuickFix(createThreadExecutorQuickFix(callExpression, String.format(THREAD_RUN_SYNC_FORMAT, alias),
115+
calleeDottedName.get(), ANYIO_QUICK_FIX));
116+
}
117+
}
118+
}
119+
120+
private static PythonQuickFix createThreadExecutorQuickFix(CallExpression callExpression,
121+
String executorPrefix,
122+
String calleeDottedName,
123+
String message) {
124+
return PythonQuickFix.newQuickFix(message)
125+
.addTextEdit(TextEditUtils.replace(callExpression.callee(), "await " + executorPrefix))
126+
.addTextEdit(TextEditUtils.insertAfter(callExpression.leftPar(),
127+
calleeDottedName + (callExpression.arguments().isEmpty() ? "" : ", ")))
128+
.build();
63129
}
64130
}
131+

‎python-checks/src/test/java/org/sonar/python/checks/SynchronousOsCallsInAsyncCheckTest.java‎

Lines changed: 167 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,12 @@
1616
*/
1717
package org.sonar.python.checks;
1818

19+
import java.util.stream.Stream;
1920
import org.junit.jupiter.api.Test;
21+
import org.junit.jupiter.params.ParameterizedTest;
22+
import org.junit.jupiter.params.provider.Arguments;
23+
import org.junit.jupiter.params.provider.MethodSource;
24+
import org.sonar.python.checks.quickfix.PythonQuickFixVerifier;
2025
import org.sonar.python.checks.utils.PythonCheckVerifier;
2126

2227
class SynchronousOsCallsInAsyncCheckTest {
@@ -25,4 +30,166 @@ class SynchronousOsCallsInAsyncCheckTest {
2530
void test() {
2631
PythonCheckVerifier.verify("src/test/resources/checks/synchronousOsCallsInAsync.py", new SynchronousOsCallsInAsyncCheck());
2732
}
33+
34+
static Stream<Arguments> quickFixTestCases() {
35+
return Stream.of(
36+
// Trio test
37+
Arguments.of(
38+
"Wrap with \"await trio.thread.executor\".",
39+
"""
40+
import trio
41+
import os
42+
43+
async def foo():
44+
os.waitpid(123, 0)
45+
""",
46+
"""
47+
import trio
48+
import os
49+
50+
async def foo():
51+
await trio.to_thread.run_sync(os.waitpid, 123, 0)
52+
"""
53+
),
54+
// Anyio test
55+
Arguments.of(
56+
"Wrap with \"await anyio.thread.executor\".",
57+
"""
58+
import anyio
59+
import os
60+
61+
async def foo():
62+
os.waitpid(123, 0)
63+
""",
64+
"""
65+
import anyio
66+
import os
67+
68+
async def foo():
69+
await anyio.to_thread.run_sync(os.waitpid, 123, 0)
70+
"""
71+
),
72+
// Test with keyword arguments
73+
Arguments.of(
74+
"Wrap with \"await trio.thread.executor\".",
75+
"""
76+
import trio
77+
import os
78+
79+
async def foo():
80+
os.waitpid(pid=123, options=0)
81+
""",
82+
"""
83+
import trio
84+
import os
85+
86+
async def foo():
87+
await trio.to_thread.run_sync(os.waitpid, pid=123, options=0)
88+
"""
89+
),
90+
// Test with alias
91+
Arguments.of(
92+
"Wrap with \"await trio.thread.executor\".",
93+
"""
94+
import trio as t
95+
import os
96+
97+
async def foo():
98+
os.waitpid(123, 0)
99+
""",
100+
"""
101+
import trio as t
102+
import os
103+
104+
async def foo():
105+
await t.to_thread.run_sync(os.waitpid, 123, 0)
106+
"""
107+
)
108+
);
109+
}
110+
111+
@ParameterizedTest
112+
@MethodSource("quickFixTestCases")
113+
void quickFixTest(String expectedMessage, String before, String after) {
114+
var check = new SynchronousOsCallsInAsyncCheck();
115+
PythonQuickFixVerifier.verify(check, before, after);
116+
PythonQuickFixVerifier.verifyQuickFixMessages(check, before, expectedMessage);
117+
}
118+
119+
static Stream<Arguments> noQuickFixTestCases() {
120+
return Stream.of(
121+
// Submodule import test
122+
Arguments.of(
123+
"""
124+
import trio.lowlevel
125+
import os
126+
127+
async def foo():
128+
os.waitpid(123, 0) # Noncompliant
129+
"""
130+
),
131+
// From import test
132+
Arguments.of(
133+
"""
134+
from trio import sleep
135+
import os
136+
137+
async def foo():
138+
os.waitpid(123, 0) # Noncompliant
139+
"""
140+
),
141+
// No import test
142+
Arguments.of(
143+
"""
144+
import os
145+
146+
async def foo():
147+
os.waitpid(123, 0) # Noncompliant
148+
"""
149+
)
150+
);
151+
}
152+
153+
@ParameterizedTest
154+
@MethodSource("noQuickFixTestCases")
155+
void noQuickFixTest(String code) {
156+
var check = new SynchronousOsCallsInAsyncCheck();
157+
PythonQuickFixVerifier.verifyNoQuickFixes(check, code);
158+
}
159+
160+
@Test
161+
void multipleImportsTest() {
162+
var check = new SynchronousOsCallsInAsyncCheck();
163+
var before = """
164+
import trio
165+
import anyio
166+
import os
167+
168+
async def foo():
169+
os.waitpid(123, 0)
170+
""";
171+
172+
var afterWithTrio = """
173+
import trio
174+
import anyio
175+
import os
176+
177+
async def foo():
178+
await trio.to_thread.run_sync(os.waitpid, 123, 0)
179+
""";
180+
181+
var afterWithAnyio = """
182+
import trio
183+
import anyio
184+
import os
185+
186+
async def foo():
187+
await anyio.to_thread.run_sync(os.waitpid, 123, 0)
188+
""";
189+
190+
PythonQuickFixVerifier.verify(check, before, afterWithTrio, afterWithAnyio);
191+
PythonQuickFixVerifier.verifyQuickFixMessages(check, before,
192+
"Wrap with \"await trio.thread.executor\".",
193+
"Wrap with \"await anyio.thread.executor\".");
194+
}
28195
}

‎python-frontend/src/main/java/org/sonar/python/tree/TreeUtils.java‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -524,4 +524,33 @@ public static List<String> dottedNameToPartFqn(DottedName dottedName) {
524524
.toList();
525525
}
526526

527+
528+
/**
529+
* Extracts the string value from a name or qualified expression.
530+
* Returns empty Optional for expressions that are not simple names or chains of qualified expressions.
531+
*/
532+
public static Optional<String> stringValueFromNameOrQualifiedExpression(Expression expression) {
533+
if (expression.is(Tree.Kind.NAME)) {
534+
return Optional.of(((Name) expression).name());
535+
} else if (expression.is(Tree.Kind.QUALIFIED_EXPR)) {
536+
return extractStringValueFromQualifiedExpression((QualifiedExpression) expression);
537+
}
538+
return Optional.empty();
539+
}
540+
541+
542+
private static Optional<String> extractStringValueFromQualifiedExpression(QualifiedExpression qualifiedExpression) {
543+
String memberName = qualifiedExpression.name().name();
544+
Expression qualifier = qualifiedExpression.qualifier();
545+
546+
if (qualifier.is(Tree.Kind.NAME)) {
547+
return Optional.of(((Name) qualifier).name() + "." + memberName);
548+
} else if (qualifier.is(Tree.Kind.QUALIFIED_EXPR)) {
549+
return extractStringValueFromQualifiedExpression((QualifiedExpression) qualifier)
550+
.map(qualifiedName -> qualifiedName + "." + memberName);
551+
}
552+
553+
return Optional.empty();
554+
}
555+
527556
}

‎python-frontend/src/test/java/org/sonar/python/tree/TreeUtilsTest.java‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -688,6 +688,33 @@ void dottedNameToPart() {
688688
assertThat(result).hasSameElementsAs(List.of("a"));
689689
}
690690

691+
@Test
692+
void test_stringValueFromNameOrQualifiedExpression() {
693+
// Simple name
694+
Expression expression = lastExpression("foo");
695+
assertThat(TreeUtils.stringValueFromNameOrQualifiedExpression(expression)).contains("foo");
696+
697+
// Qualified expression
698+
expression = lastExpression("os.path");
699+
assertThat(TreeUtils.stringValueFromNameOrQualifiedExpression(expression)).contains("os.path");
700+
701+
// Nested qualified expression
702+
expression = lastExpression("os.path.join");
703+
assertThat(TreeUtils.stringValueFromNameOrQualifiedExpression(expression)).contains("os.path.join");
704+
705+
// Call expression - should return empty
706+
expression = lastExpression("foo()");
707+
assertThat(TreeUtils.stringValueFromNameOrQualifiedExpression(expression)).isEmpty();
708+
709+
// Complex qualifier - should return empty
710+
expression = lastExpression("(x + y).method");
711+
assertThat(TreeUtils.stringValueFromNameOrQualifiedExpression(expression)).isEmpty();
712+
713+
// Binary expression - should return empty
714+
expression = lastExpression("a + b");
715+
assertThat(TreeUtils.stringValueFromNameOrQualifiedExpression(expression)).isEmpty();
716+
}
717+
691718
private static boolean isOuterFunction(Tree tree) {
692719
return tree.is(Kind.FUNCDEF) && ((FunctionDef) tree).name().name().equals("outer");
693720
}
@@ -698,3 +725,4 @@ private FileInput parse(String content) {
698725
return new PythonTreeMaker().fileInput(astNode);
699726
}
700727
}
728+

0 commit comments

Comments
 (0)