Skip to content

Conversation

@mcquenji
Copy link
Owner

@mcquenji mcquenji commented Jun 15, 2025

  • fs lib
  • http lib
  • json lib
  • utf8 lib
  • date time lib
  • ascii lib
  • base64 lib

@mcquenji mcquenji added the enhancement New feature or request label Jun 15, 2025
mcquenji added 5 commits June 16, 2025 17:37
The infinite loop was caused by the catch target not being popped after entering it. This caused all exceptions thrown within the catch block to just jump to the start of the block, thus infinitley calling itself instead of just erroring out
@github-actions

This comment was marked as resolved.

@mcquenji mcquenji changed the title Enhance stdlib implement missing libraries Jun 16, 2025
@mcquenji mcquenji requested a review from Copilot June 16, 2025 23:16

This comment was marked as outdated.

@mcquenji mcquenji added the stdlib This issue is about the standard library label Jun 18, 2025
@mcquenji mcquenji linked an issue Aug 11, 2025 that may be closed by this pull request
- DateTime lib is still incomplete

BREAKING: Structs are now generic and require a toDart and fromDart function upon creation (does not apply to shallow structs)
@mcquenji mcquenji requested a review from Copilot August 17, 2025 01:11
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements several missing standard library modules for the dscript language, introducing comprehensive support for filesystem operations, HTTP requests, JSON handling, UTF-8 encoding, Base64 encoding, and utility functions for lists, maps, and dynamic values.

  • Refactored the Struct type system to support typed conversion between DSL and Dart objects
  • Implemented new standard library modules: fs, http, json, utf8, base64, list, map, and dynamic
  • Enhanced the RuntimeBinding system with middleware support and improved parameter handling
  • Added comprehensive documentation and custom linting rules

Reviewed Changes

Copilot reviewed 47 out of 51 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
lib/src/types.dart Enhanced Struct type with generic support and predefined structs for HTTP responses, JSON, DateTime, and Duration
lib/src/stdlib/*.dart Implemented new standard library modules with comprehensive functionality
lib/src/bindings.dart Refactored RuntimeBinding with middleware support and improved parameter validation
lib/src/contract_builder.dart Updated contract builder to support new parameter naming and return type specification
pubspec.yaml Added new dependencies (dio, custom_lint) and removed unused stack dependency
docs/ Updated documentation with comprehensive standard library reference
custom_lints/ Added custom linting rules to prevent type.toString() usage in web builds

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

name: 'add',
function: (List<dynamic> list, dynamic element) => list.add(element),
positionalParams: {
'list': ListType(elementType: const DynamicType()),
Copy link

Copilot AI Aug 17, 2025

Choose a reason for hiding this comment

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

The 'add' binding is missing the 'element' parameter in positionalParams. This will cause a runtime error when validating arguments since the function expects 2 parameters but only 1 is declared.

Suggested change
'list': ListType(elementType: const DynamicType()),
'list': ListType(elementType: const DynamicType()),
'element': const DynamicType(),

Copilot uses AI. Check for mistakes.
name: 'remove',
function: (List<dynamic> list, dynamic element) => list.remove(element),
positionalParams: {
'list': ListType(elementType: const DynamicType()),
Copy link

Copilot AI Aug 17, 2025

Choose a reason for hiding this comment

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

The 'remove' binding is missing the 'element' parameter in positionalParams. This will cause a runtime error when validating arguments since the function expects 2 parameters but only 1 is declared.

Suggested change
'list': ListType(elementType: const DynamicType()),
'list': ListType(elementType: const DynamicType()),
'element': const DynamicType(),

Copilot uses AI. Check for mistakes.
value,
'value',
'Value must be a collection or string to get length.',
);
Copy link

Copilot AI Aug 17, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider using a more efficient type check pattern like 'value.length' directly with try-catch, which would avoid multiple type checks and leverage Dart's dynamic dispatch.

Suggested change
);
try {
return value.length;
} catch (e) {
throw ArgumentError.value(
value,
'value',
'Value must have a length property.',
);
}

Copilot uses AI. Check for mistakes.
store(frame, index, stack.removeLast());

// pop this catch target to avoid infinite loops if the catch block throws again
if (catchTargets.isNotEmpty) catchTargets.removeLast();
Copy link

Copilot AI Aug 17, 2025

Choose a reason for hiding this comment

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

This comment and logic suggest removing catch targets unconditionally during exception handling, but this could break proper exception handling flow. The catch target should only be removed when the catch block completes successfully, not during exception processing.

Suggested change
if (catchTargets.isNotEmpty) catchTargets.removeLast();
// Do not remove the catch target here; it should only be removed when the catch block completes successfully (in endTry).

Copilot uses AI. Check for mistakes.

static Map<String, dynamic> _shallowFromDart<T>(T obj) {
throw UnimplementedError(
r'Cannot convert Dart object to shallow struct. Use `$Type.lookup` to get the resolved struct and call `fromDart` on it.');
Copy link

Copilot AI Aug 17, 2025

Choose a reason for hiding this comment

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

The error message references '$Type.lookup' method which may not exist or be accessible to users. Consider providing a more actionable error message with the correct method name or approach.

Suggested change
r'Cannot convert Dart object to shallow struct. Use `$Type.lookup` to get the resolved struct and call `fromDart` on it.');
'Cannot convert shallow struct to Dart object. Ensure you are using a fully resolved struct and call `toDart` on it.');
}
static Map<String, dynamic> _shallowFromDart<T>(T obj) {
throw UnimplementedError(
'Cannot convert Dart object to shallow struct. Ensure you are using a fully resolved struct and call `fromDart` on it.');

Copilot uses AI. Check for mistakes.
final contents = dir.listSync();
return contents
.whereType<File>()
.map((file) => file.path)
Copy link

Copilot AI Aug 17, 2025

Choose a reason for hiding this comment

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

The function documentation says it 'Lists the contents of a directory' but the implementation only returns files, not directories. Either update the documentation or include directories in the result.

Suggested change
.map((file) => file.path)
.map((entity) => entity.path)

Copilot uses AI. Check for mistakes.

/// Default structs defined within the language.
static final defaults = [error];
static final defaults = [error, httpResponse, json, duration, dateTime];
Copy link

Copilot AI Aug 17, 2025

Choose a reason for hiding this comment

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

The 'mapEntry' struct is defined but not included in the defaults list, which means it won't be available for type resolution during compilation and runtime.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request stdlib This issue is about the standard library

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[feature] .copy() for collections

2 participants