Skip to content
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

Incorrect CustomStringConvertible implementations #301

Closed
2 tasks done
lorentey opened this issue Jul 6, 2023 · 3 comments
Closed
2 tasks done

Incorrect CustomStringConvertible implementations #301

lorentey opened this issue Jul 6, 2023 · 3 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@lorentey
Copy link
Member

lorentey commented Jul 6, 2023

OrderedDictionary.description does not use the same output style as Dictionary -- it prints its keys and values by calling String(describing:), rather than the expected String(reflecting:). This allows the contents of keys and values to confusingly intermix with the syntax of the description's structural output.

Information

  • Package version: release/1.0, release/1.1, main
  • Platform version: n.a.
  • Swift version: 5.9

Checklist

  • If possible, I've reproduced the issue using the main branch of this package.
  • I've searched for existing GitHub issues.

Steps to Reproduce

    let d: OrderedDictionary = [
      "1: 2, 3": 4,
      "5: 6, 7": 8,
    ]
    print(d)

Expected behavior

I expect the code to print output that distinguishes keys from values.

Actual behavior

The sample code prints the following output:

[1: 2, 3: 4, 5: 6, 7: 8]

This is extremely confusing -- the values of the keys are allowed to interfere with the syntactic punctuation in the dictionary's structured description.

In contrast, the standard Dictionary.description uses debugDescription to print its contents, preventing such confusion:

    let d: Dictionary = [
      "1: 2, 3": 4,
      "5: 6, 7": 8,
    ]
    print(d) // ⟹ ["1: 2, 3": 4, "5: 6, 7": 8]

The output is not exactly crystal clear, but it is at least possible to e.g. count the number of elements in the dictionary.

@lorentey lorentey added the bug Something isn't working label Jul 6, 2023
@lorentey lorentey self-assigned this Jul 6, 2023
@lorentey
Copy link
Member Author

lorentey commented Jul 6, 2023

The descriptions are generated by code that is shared across all container types in this package (_dictionaryDescription and _arrayDescription), so they are all affected by this problem.

@lorentey
Copy link
Member Author

lorentey commented Jul 6, 2023

I think it'd be a good idea to also drop the type names from debugDescription as part of resolving this -- generally it is a bad idea to do that as type information is readily available elsewhere (from context, for example).

Given the use of debugDesctiption in Array/Set/Dictionary's description implementation, best practice is to keep description and debugDescription largely the same, except for escaping things that could potentially interfere with surrounding syntactic punctuation -- precisely like String does.

lorentey added a commit to lorentey/swift-collections that referenced this issue Jul 7, 2023
- For CustomStringConvertible, `description` should print elements using `String(reflecting:)` rather than `String(describing:)`, to avoid element output from interfering with syntactically relevant punctuation within structured list displays.
- To make this work without making displays overly verbose, make `debugDescription` mostly the same as `description`, except for escaping raw string displays when and if needed.
- Avoid printing type names in `debugDescription` — it is generally not helpful, as the type is already easily accessible elsewhere. Including it makes for overly verbose displays when the type serves as an Element of a collection.
- Avoid having `debugDescription` return the empty string — it can be confusing when such a description appears in list displays.

Resolves apple#301.
@lorentey lorentey added this to the 1.1.0 milestone Jul 7, 2023
@lorentey
Copy link
Member Author

lorentey commented Jul 7, 2023

Resolved in #302. OrderedDictionary (and all other collections in the package) now follow the practices of the stdlib.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant