Skip to content

Commit 1aa2570

Browse files
aristidispmeta-codesync[bot]
authored andcommitted
Fix AllowUnsafeNonSealedKeyType annotation placement for same-line function params
Summary: When a function parameter needing thrift.AllowUnsafeNonSealedKeyType shared a line with the function signature, the annotation was incorrectly placed before the entire function line. Add annotate_function_param() that detects this case and breaks the line after the preceding ( or , separator, placing the annotation on its own indented line before the parameter. Reviewed By: hchokshi Differential Revision: D95133891 fbshipit-source-id: 7bbca99608804a5346569854dedde374a29e99ca
1 parent 9555ddb commit 1aa2570

File tree

2 files changed

+132
-1
lines changed

2 files changed

+132
-1
lines changed

third-party/thrift/src/thrift/compiler/codemod/annotate_allow_unsafe_non_sealed_key_type.cc

Lines changed: 53 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ class AnnotateAllowUnsafeNonSealedKeyType final {
8787
for (const t_field& param : func.params().fields()) {
8888
const t_type& type = param.type().deref();
8989
if (needs_annotation(type, param)) {
90-
annotate_node(param.type().src_range().begin);
90+
annotate_function_param(param, func);
9191
any_annotated = true;
9292
}
9393
}
@@ -138,6 +138,58 @@ class AnnotateAllowUnsafeNonSealedKeyType final {
138138
source_manager_.get_text_range(line_leading_whitespace),
139139
kAnnotation)});
140140
}
141+
142+
/**
143+
* Adds the `@thrift.AllowUnsafeNonSealedKeyType` annotation before a
144+
* function parameter.
145+
*
146+
* If the parameter is on its own line, delegates to `annotate_node`.
147+
* If the parameter shares a line with the function signature (or other
148+
* parameters), breaks the line after the preceding `(` or `,` and inserts
149+
* the annotation on a new line before the parameter. The indentation is
150+
* computed from the function's line (+ 2 spaces) so that all parameters
151+
* align consistently.
152+
*/
153+
void annotate_function_param(const t_field& param, const t_function& func) {
154+
source_location param_loc = param.src_range().begin;
155+
source_range line_ws = file_manager_.get_line_leading_whitespace(param_loc);
156+
157+
size_t param_offset = file_manager_.to_offset(param_loc);
158+
size_t ws_end_offset = file_manager_.to_offset(line_ws.end);
159+
160+
// If param is on its own line, the existing logic works.
161+
if (ws_end_offset >= param_offset) {
162+
annotate_node(param_loc);
163+
return;
164+
}
165+
166+
maybe_add_include();
167+
168+
// Compute indent from the function's line so all params align at the
169+
// same level, regardless of which line the param originally appeared on.
170+
source_range func_line_ws =
171+
file_manager_.get_line_leading_whitespace(func.src_range().begin);
172+
std::string func_indent(source_manager_.get_text_range(func_line_ws));
173+
std::string param_indent = func_indent + " ";
174+
175+
// Scan backwards from param_loc over horizontal whitespace to find
176+
// where the preceding '(' or ',' ends.
177+
std::string_view old_content = file_manager_.old_content();
178+
size_t scan_back = param_offset;
179+
while (scan_back > 0 &&
180+
(old_content[scan_back - 1] == ' ' ||
181+
old_content[scan_back - 1] == '\t')) {
182+
--scan_back;
183+
}
184+
185+
// Replace whitespace between separator and param with
186+
// line break + annotation + line break + indent.
187+
file_manager_.add(
188+
{.begin_pos = scan_back,
189+
.end_pos = param_offset,
190+
.new_content = fmt::format(
191+
"\n{}{}\n{}", param_indent, kAnnotation, param_indent)});
192+
}
141193
};
142194

143195
} // namespace

third-party/thrift/src/thrift/compiler/codemod/annotate_allow_unsafe_non_sealed_key_type_test.py

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -325,3 +325,82 @@ def test_typedef_indirection_not_annotated(self):
325325
}
326326
""",
327327
)
328+
329+
def test_function_param_same_line(self):
330+
self._check(
331+
before="""\
332+
include "thrift/annotation/thrift.thrift"
333+
334+
struct NonSealedStruct {}
335+
336+
service MyService {
337+
void myFunc(1: set<NonSealedStruct> param);
338+
}
339+
""",
340+
after="""\
341+
include "thrift/annotation/thrift.thrift"
342+
343+
struct NonSealedStruct {}
344+
345+
service MyService {
346+
void myFunc(
347+
@thrift.AllowUnsafeNonSealedKeyType
348+
1: set<NonSealedStruct> param);
349+
}
350+
""",
351+
)
352+
353+
def test_function_param_same_line_multiple_params(self):
354+
self._check(
355+
before="""\
356+
include "thrift/annotation/thrift.thrift"
357+
358+
struct NonSealedStruct {}
359+
360+
service MyService {
361+
void myFunc(1: i32 x, 2: set<NonSealedStruct> param);
362+
}
363+
""",
364+
after="""\
365+
include "thrift/annotation/thrift.thrift"
366+
367+
struct NonSealedStruct {}
368+
369+
service MyService {
370+
void myFunc(1: i32 x,
371+
@thrift.AllowUnsafeNonSealedKeyType
372+
2: set<NonSealedStruct> param);
373+
}
374+
""",
375+
)
376+
377+
def test_function_param_same_line_multiple_non_sealed_params(self):
378+
self._check(
379+
before="""\
380+
include "thrift/annotation/thrift.thrift"
381+
382+
struct NonSealedStruct {}
383+
384+
service MyService {
385+
void myFunc(1: set<NonSealedStruct> p1,
386+
2: set<NonSealedStruct> p2, 3: map<NonSealedStruct, i32> p3
387+
);
388+
}
389+
""",
390+
after="""\
391+
include "thrift/annotation/thrift.thrift"
392+
393+
struct NonSealedStruct {}
394+
395+
service MyService {
396+
void myFunc(
397+
@thrift.AllowUnsafeNonSealedKeyType
398+
1: set<NonSealedStruct> p1,
399+
@thrift.AllowUnsafeNonSealedKeyType
400+
2: set<NonSealedStruct> p2,
401+
@thrift.AllowUnsafeNonSealedKeyType
402+
3: map<NonSealedStruct, i32> p3
403+
);
404+
}
405+
""",
406+
)

0 commit comments

Comments
 (0)