rust-analyzer / rowan

Apache License 2.0
704 stars 58 forks source link

Prevent use of SyntaxNodePtr and AstPtr on mutable trees #151

Closed Technohacker closed 4 months ago

Technohacker commented 1 year ago

Both of these use source code locations to identify the node, which can get invalidated by syntax tree mutations. This commit adds assertions to prevent their use, and adds documentation to inform users of the issue

Fixes #150

Technohacker commented 1 year ago

There's an added is_mutable() method for cursor::SyntaxNode and api::SyntaxNode which was needed for use in the API module. It may also be useful as a public API in case users wish to check if a tree is mutable

There's also 2 tests added to ensure this panic behaviour, but I've had to make a custom impl of Language to do so

jelmer commented 4 months ago

@Technohacker this appears conflicted now

Technohacker commented 4 months ago

Ah I can get that resolved in a few mins

Technohacker commented 4 months ago

All done :+1: I've still assumed mutable trees should cause panics since I figured it's a programming mistake to use SyntaxNodePtrs on mutable trees. Should it be an Option return like the missing node case instead?

Also not sure what's going on with the CI, could you have a check?

jelmer commented 4 months ago

Hmm, it appears to fail in master for the same reasons. https://github.com/rust-analyzer/rowan/actions/runs/9030330328/job/24814372618

Veykril commented 4 months ago

Well that's a fun new lint we trigger

Veykril commented 4 months ago

Thanks