dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.09k stars 1.56k forks source link

[pkg/meta]: Add `@AvoidOutsideOf` #49461

Open matanlurey opened 2 years ago

matanlurey commented 2 years ago

There are methods that are generally unsafe to use, and you'd want to avoid using unintentionally.

For example, on the web, some methods and getters/setters cause forced synchronous layout.

Now, once option would be creating two custom annotations (@avoidSynchronousLayout, @allowSynchronousLayout), and go and update libraries and add the former annotation on some library APIs, and the latter annotation on other APIs:

import 'package:meta/meta.dart';

abstract class Element {
  @avoidSynchronousLayout
  external int get offsetWidth;
}

// ...

void unsafe(Element e) {
  // LINT: Avoid synchronous layout
  print(e.offsetWidth);
}

@allowSynchronousLayout
void unsafeExplicitly(Element e) {
  // OK
  print(e.offsetWidth);
}

However, that seems pretty specialized for something that actually could be used by different libraries/frameworks (similar to @useResult).

What if we instead created two classes:

/// A class that produces annotations indicating an APIs is unsafe, and should be used carefully.
///
/// Tools, such as the analyzer, can provide feedback if:
///
/// - the annotation is associated with anything other than an API
/// - an API is accessed that is marked by this annotation, and does not occur in <yada yada>
///
/// ```
/// class _AllowSynchronousLayout extends AllowOtherwise {}
/// const allowSynhronousLayout = _AllowSynchronousLayout();
/// const avoidSynchronousLayout = AvoidOutsideOf(allowSynchronousLayout);
///
///  class Element {
///    @avoidSynchronousLayout
///    void exampleUnsafeMethod() {}
///  }
/// ```
class AvoidOutsideOf {
  final AllowOtherwise annotation;

  const AvoidOutsideOf(this.annotation);
}

abstract class AvoidOutsideOf {}

(There might be a cuter way to design the classes, but really I'm just interested in the end result, which is the ability to define unsafe APIs and opt-ins for using those unsafe APIs)

/cc @srawlins

bwilkerson commented 2 years ago

I'm guessing that the second (abstract) class was intended to be AllowOtherwise.

I might suggest that we define AllowOtherwise to be concrete with a field whose value is the name of the annotation for which the instance is the value. In other words:

class AllowOtherwise {
  final String annotationName;
  const AllowOtherwise(this.annotationName);
}

That would allow for better messages, such as "The method 'exampleUnsafeMethod' should not be invoked except in methods annotated with 'allowSynchronousLayout'."

I'm not sure how useful these annotations would be for Flutter developers, so I might propose that we prototype them internally first.

matanlurey commented 2 years ago

to be concrete with a field whose value is the name of the annotation

I would prefer not to use stringly-typed annotations :)

I'm not sure how useful these annotations would be for Flutter developers

This is a general use annotation, the example I gave was specific to web (though, still impacts Flutter web).

bwilkerson commented 2 years ago

@goderbauer For visibility.

jonahwilliams commented 2 years ago

One potential use-case I can see for this in Flutter land is making sure certain blocking IO methods are not called from build/layout methods, since that can stall rendering - while still allowing them in other cases, since using blocking io is safe to do from an isolate/compute:

example:

bad:

class FooWidget extends StatelessWidget {

  Widget build(BuildContext context) {
     return Image.memory(File('data.png').readAsBytesSync()); // BAD!
  }
}

ok:

class FooWidgetState extends State<FooWidget> {
  Uint8List? data;

  Uint8List _loadData(_) {
    return File('data.png').readAsBytesSync(); // OK
   }

  initState() {
    super.initState();
    compute(_loadData, null).then((bytes) {
      setState(() => data = bytes); 
     });
  }

  Widget build(BuildContext context) {
     ...
  }
}

not including stuff like existsSync(), which is generally always OK