Skip to content

[pigeon] add error on empty class #11933

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

Open
tarrinneal wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[pigeon] add error on empty class #11933

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions packages/pigeon/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 27.1.1

* Fixes validation bug where empty classes without `sealed` keyword were not correctly flagged with an error.

## 27.1.0

* [swift] Adds `CaseIterable` conformance to generated enums.
Expand Down
2 changes: 1 addition & 1 deletion packages/pigeon/lib/src/generator_tools.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import 'generator.dart';
/// The current version of pigeon.
///
/// This must match the version in pubspec.yaml.
const String pigeonVersion = '27.1.0';
const String pigeonVersion = '27.1.1';

/// Default plugin package name.
const String defaultPluginPackageName = 'dev.flutter.pigeon';
Expand Down
99 changes: 48 additions & 51 deletions packages/pigeon/lib/src/pigeon_lib_internal.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -619,6 +619,25 @@ List<Error> _validateAst(Root root, String source) {
),
);
}
if (classDefinition.isSealed) {
if (classDefinition.fields.isNotEmpty) {
result.add(
Error(message: 'Sealed class: "${classDefinition.name}" must not contain fields.'),
);
}
}
if (classDefinition.fields.isEmpty && !classDefinition.isSealed) {
result.add(
Error(message: 'Class: "${classDefinition.name}" must contain fields or be sealed.'),
);
}
Comment on lines +629 to +633

@gemini-code-assist gemini-code-assist Bot Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

In sealed class hierarchies (union types), it is a very common and idiomatic pattern to have empty subclasses to represent states with no associated data (for example, a Loading or Empty state in a Result or Option union).

Currently, the empty class check flags any non-sealed class with no fields as an error. This will incorrectly reject valid empty subclasses of sealed classes.

To support idiomatic sealed class unions, we should allow empty classes if they are subclasses (i.e., superClassName != null).

Suggested change
if (classDefinition.fields.isEmpty && !classDefinition.isSealed) {
result.add(
Error(message: 'Class: "${classDefinition.name}" must contain fields or be sealed.'),
);
}
if (classDefinition.fields.isEmpty && !classDefinition.isSealed && classDefinition.superClassName == null) {
result.add(
Error(message: 'Class: "${classDefinition.name}" must contain fields or be sealed.'),
);
}

@stuartmorgan-g stuartmorgan-g Jun 18, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is Gemini correct here, or is isSealed true for subclasses of a sealed class?

@stuartmorgan-g stuartmorgan-g Jun 18, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

(Either way, we should have a test of this scenario.)

if (classDefinition.superClass != null) {
if (!classDefinition.superClass!.isSealed) {
result.add(
Error(message: 'Child class: "${classDefinition.name}" must extend a sealed class.'),
);
}
}
Comment on lines +634 to +640

@gemini-code-assist gemini-code-assist Bot Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

If a class extends an unknown or undefined class (e.g., due to a typo or because the parent class is missing from the file), classDefinition.superClass will be null while classDefinition.superClassName is non-null. In this case, the current check classDefinition.superClass != null is skipped entirely, allowing the undefined inheritance to pass validation silently.

To prevent this silent bypass, we should check if superClassName is non-null, and flag an error if superClass is either null (meaning the parent class was not found/defined) or not sealed.

Suggested change
if (classDefinition.superClass != null) {
if (!classDefinition.superClass!.isSealed) {
result.add(
Error(message: 'Child class: "${classDefinition.name}" must extend a sealed class.'),
);
}
}
if (classDefinition.superClassName != null) {
if (classDefinition.superClass == null || !classDefinition.superClass!.isSealed) {
result.add(
Error(message: 'Child class: "${classDefinition.name}" must extend a sealed class.'),
);
}
}

for (final NamedType field in getFieldsInSerializationOrder(classDefinition)) {
final String? matchingPrefix = _findMatchingPrefixOrNull(
field.name,
Expand All @@ -644,26 +663,6 @@ List<Error> _validateAst(Root root, String source) {
),
);
}
if (classDefinition.isSealed) {
if (classDefinition.fields.isNotEmpty) {
result.add(
Error(
message: 'Sealed class: "${classDefinition.name}" must not contain fields.',
lineNumber: _calculateLineNumberNullable(source, field.offset),
),
);
}
}
if (classDefinition.superClass != null) {
if (!classDefinition.superClass!.isSealed) {
result.add(
Error(
message: 'Child class: "${classDefinition.name}" must extend a sealed class.',
lineNumber: _calculateLineNumberNullable(source, field.offset),
),
);
}
}
}
}

Expand Down Expand Up @@ -1805,22 +1804,21 @@ class RootBuilder extends dart_ast_visitor.RecursiveAstVisitor<Object?> {
lineNumber: calculateLineNumber(source, node.offset),
),
);
} else {
final dart_ast.TypeArgumentList? typeArguments = type.typeArguments;
final String name = node.fields.variables[0].name.lexeme;
final field = NamedType(
type: TypeDeclaration(
baseName: _getNamedTypeQualifiedName(type),
isNullable: type.question != null,
typeArguments: _typeAnnotationsToTypeArguments(typeArguments),
),
name: name,
offset: node.offset,
defaultValue: _currentClassDefaultValues[name],
documentationComments: _documentationCommentsParser(node.documentationComment?.tokens),
);
_currentClass!.fields.add(field);
}
final dart_ast.TypeArgumentList? typeArguments = type.typeArguments;
final String name = node.fields.variables[0].name.lexeme;
final field = NamedType(
type: TypeDeclaration(
baseName: _getNamedTypeQualifiedName(type),
isNullable: type.question != null,
typeArguments: _typeAnnotationsToTypeArguments(typeArguments),
),
name: name,
offset: node.offset,
defaultValue: _currentClassDefaultValues[name],
documentationComments: _documentationCommentsParser(node.documentationComment?.tokens),
);
_currentClass!.fields.add(field);
} else {
_errors.add(
Error(
Expand Down Expand Up @@ -1969,23 +1967,22 @@ class RootBuilder extends dart_ast_visitor.RecursiveAstVisitor<Object?> {
lineNumber: calculateLineNumber(source, node.offset),
),
);
} else {
final dart_ast.TypeArgumentList? typeArguments = type.typeArguments;
(_currentApi as AstProxyApi?)!.fields.add(
ApiField(
type: TypeDeclaration(
baseName: _getNamedTypeQualifiedName(type),
isNullable: type.question != null,
typeArguments: _typeAnnotationsToTypeArguments(typeArguments),
),
name: node.fields.variables[0].name.lexeme,
isAttached: _hasMetadata(node.metadata, 'attached') || isStatic,
isStatic: isStatic,
offset: node.offset,
documentationComments: _documentationCommentsParser(node.documentationComment?.tokens),
),
);
}
final dart_ast.TypeArgumentList? typeArguments = type.typeArguments;
(_currentApi as AstProxyApi?)!.fields.add(
ApiField(
type: TypeDeclaration(
baseName: _getNamedTypeQualifiedName(type),
isNullable: type.question != null,
typeArguments: _typeAnnotationsToTypeArguments(typeArguments),
),
name: node.fields.variables[0].name.lexeme,
isAttached: _hasMetadata(node.metadata, 'attached') || isStatic,
isStatic: isStatic,
offset: node.offset,
documentationComments: _documentationCommentsParser(node.documentationComment?.tokens),
),
);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion packages/pigeon/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ name: pigeon
description: Code generator tool to make communication between Flutter and the host platform type-safe and easier.
repository: https://github.com/flutter/packages/tree/main/packages/pigeon
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+pigeon%22
version: 27.1.0 # This must match the version in lib/src/generator_tools.dart
version: 27.1.1 # This must match the version in lib/src/generator_tools.dart

environment:
sdk: ^3.10.0
Expand Down
21 changes: 20 additions & 1 deletion packages/pigeon/test/pigeon_lib_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,23 @@ abstract class Api {
expect(results.errors[0].message, contains('dynamic'));
});

test('empty class error', () {
const source = '''
class EmptyClass {}

@HostApi()
abstract class Api {
void foo(EmptyClass empty);
}
''';
final ParseResults results = parseSource(source);
expect(results.errors, hasLength(1));
expect(
results.errors[0].message,
contains('Class: "EmptyClass" must contain fields or be sealed.'),
);
});

test('Only allow one api annotation', () {
const source = '''
@HostApi()
Expand Down Expand Up @@ -1703,7 +1720,9 @@ abstract class EventChannelApi {
group('sealed inheritance validation', () {
test('super class must be sealed', () {
const code = '''
class DataClass {}
class DataClass {
int? someField;
}
class ChildClass extends DataClass {
ChildClass(this.input);
int input;
Expand Down
Loading