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

convert and preview commands crash on certain Markdown pages #678

Closed
2 tasks done
MaxDesiatov opened this issue Aug 2, 2023 · 15 comments · Fixed by #683, apple/swift-markdown#151 or #733
Closed
2 tasks done

convert and preview commands crash on certain Markdown pages #678

MaxDesiatov opened this issue Aug 2, 2023 · 15 comments · Fixed by #683, apple/swift-markdown#151 or #733
Labels
bug Something isn't working

Comments

@MaxDesiatov
Copy link
Member

Description

When rendering Swift compiler documentation the diagnostics formatting code crashes on certain Markdown pages.

Checklist

  • If possible, I've reproduced the issue using the main branch of this package.
  • This issue hasn't been addressed in an existing GitHub issue.

Expected Behavior

No crashes.

Actual behavior

Swift/StringCharacterView.swift:158: Fatal error: String index is out of bounds

Steps To Reproduce

  1. Clone https://github.com/apple/swift.git
  2. Install a recent development snapshot of Swift, I'm using swift-DEVELOPMENT-SNAPSHOT-2023-07-23-a.
  3. Run DocC from this snapshot this way (assuming swift is the clone from step 1):
docc preview --allow-arbitrary-catalog-directories swift/docs

Swift-DocC Version Information

swift-DEVELOPMENT-SNAPSHOT-2023-07-23-a

Swift Compiler Version Information

Apple Swift version 5.9-dev (LLVM 9b562f55c38e378, Swift b4ee68bd37c4f7d)
Target: arm64-apple-macosx14.0
@MaxDesiatov MaxDesiatov added the bug Something isn't working label Aug 2, 2023
@franklinsch
Copy link
Member

The code that crashes is in the new diagnostics formatter (#535). cc @arthurcro if you have any ideas 🙂

@arthurcro
Copy link
Contributor

The code that crashes is in the new diagnostics formatter (#535). cc @arthurcro if you have any ideas 🙂

Thank you for the heads up, I’ll take a look asap! 🙂

@arthurcro
Copy link
Contributor

Hi! 🙂I have identified the issue and I have found a proper fix.
The crashes were caused by poor UTF-8 encoding handling while highlighting a source line using the diagnostic source range. I will open a pull request with the fix and several tests covering the issue shortly!

Thanks again for reporting this! @MaxDesiatov cc @franklinsch

@MaxDesiatov
Copy link
Member Author

MaxDesiatov commented Aug 11, 2023

@arthurcro this is still reproducible on main with f70b3ea, I'm reopening the issue

Screenshot 2023-08-11 at 11 54 29

@arthurcro
Copy link
Contributor

@arthurcro this is still reproducible on main with f70b3ea, I'm reopening the issue

Thank you for reopening the issue and apologies for the inconvenience.
The crash is surprising, I did not face any crashes when I tested it after implementing the fix. Did you change anything this time compared to the steps mentioned in the Steps to reproduce ?

@MaxDesiatov
Copy link
Member Author

I didn't, potentially more docs were published in the docs directory in question. But this crash looked similar if it wasn't the same as the original issue, so I felt that reopening this one would work better than creating a new one.

@arthurcro
Copy link
Contributor

Hi! 😄 I'm unfortunately unable to reproduce the issue on main with the latest docs from https://github.com/apple/swift.
@franklinsch @d-ronnqvist could you check if you can reporduce if you have some time? 😊

@MaxDesiatov
Copy link
Member Author

Reproduced again with main DocC, checkout this commit with the docs: apple/swift@1de261b

@arthurcro
Copy link
Contributor

Great! I can reproduce with this commit, thank you! 🙇🏻

@arthurcro
Copy link
Contributor

Hi! 👋 After some investigation, I've identified what seems to be the underlying cause of the problem.

Interestingly, the issue doesn't seem to be directly caused by the improved default diagnostic formatter, even though the formatter does highlight it.

While I might not have captured all the details accurately, here's my understanding so far:

When using:

`` my content ``

DocC will see this as a symbol link.

For this specific crash, when using:

  for the target platform and verify diagnostics, like ``swift -frontend -typecheck -verify
%s``

DocC will try to resolve this symbol link as a TopicReference but it will eventually fail.

if let resolved = resolveAbsoluteSymbolLink(unresolvedDestination: destination, elementRange: symbolLink.range) {

To resolve the symbol link and eventually emit a diagnostic for this resolution error, DocC uses the symbol link SourceRange provided by MarkupData.swift from swift-markdown.

This source range is used as the diagnostic's source range in case of resolution error. It's then used by the default diagnostic formatter to highlight the unresolved reference. If the source range does not match the actual content of the source file, it crashes.

If we look at your crash, we get a SourceRange 253:56-253:97 while if we look at the source file content, we should be getting 253:56-254:5. It seems we are getting an incorrect source range for some symbol links.

I hope that made sense.
@d-ronnqvist I'm not sure how to proceed to fix this issue. Could you advice possible next steps? 🙇🏻

@Kyle-Ye
Copy link
Collaborator

Kyle-Ye commented Aug 19, 2023

Thanks for pointing it out. Maybe you can help file up a issue with some minimum reproducible input to Swift-Markdown so that I can help to look at it on the Markdown module side.

QuietMisdreavus should have some better insights on such problem. But it looks like she is busy now and will back September 18.

Also to know that there are several other issues about SourceRange parsing like apple/swift-markdown#71. I have tried to fix it, but the CodeReview process is pending for some unknown reason.

@Kyle-Ye
Copy link
Collaborator

Kyle-Ye commented Sep 26, 2023

I'm also frequently encountered this locally. This should definitely be a block issue for release/5.10 since it may crash a lot of downstream user.

It can be easily reproduced on main with Apple's SlothCreator demo Package

Maybe we can discuss it in next DocC workgroup. cc @franklinsch @QuietMisdreavus

IMO, We should either fix it before 5.10 release or revert relevant PR until we fix the issue.

@Kyle-Ye
Copy link
Collaborator

Kyle-Ye commented Oct 5, 2023

When trying for a fix for the markdown bug. I found the root of my crash is actually another bug in swift-docc. I'll file it later.

I changed the eat(_:quantity:) signature of SlothCreator. And it produced 2 duplicated diagnostic. One is right in the extension file. Another is not. And they both share the same diagnosticRange 21:9..<21:25. So a crash will happen on DiagnosticConsoleWriter

image image

@Kyle-Ye
Copy link
Collaborator

Kyle-Ye commented Oct 5, 2023

I hope that made sense. @d-ronnqvist I'm not sure how to proceed to fix this issue. Could you advice possible next steps? 🙇🏻

For the problem you found, I've open a PR to fix it. apple/swift-markdown#151 cc @arthurcro

And there is another crash cause, I've opened a new issue on swift-docc to track it. #729

@Kyle-Ye
Copy link
Collaborator

Kyle-Ye commented Oct 25, 2023

Both bugfix PR is merged into main. The cherry-pick to release/5.10 is still in progress.

If you are still encountering such crash on the latest toolchain, feel free to reopen the issue. @MaxDesiatov

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
4 participants